Header file for the USB video driver (easy)

[Karvjorm]

I have here an USB_video.h file for headers/os/drivers/usb

I do not actually know, how to make this patching with the new files :)

I checked USB_audio.h and tried to use it as a model. There are still only two struct definitions. More are needed, but I thought that evaluation phase would be better to do now. For example,

Br, Karvjorm

[mmlr]

Interesting. Got two remarks though:

For struct usb_video_processing_unit_descriptor I'd rather see the Dx named with what they are, as it looks like they all have a specific and fixed meaning. I'd rather use a native type like uint32 instead of the unsigned for these bitfields as well. Using the BITFIELD macros might make sense considering endianness awareness of those. Also I'm not sure the anonymous structs work with gcc2.

There are some stray comments in the "Video Interface Subclass Codes" enum.

Otherwise it looks good, thanks for looking into it.

[Karvjorm]

Thanks, I will change those Dx names to more descriptive.

I noticed that all those header files (USB_audio.h, USB_hid.h, USB_midi.h) use anonymous structs so I have used the same model.

Also, in USB_midi.h there are "uint8 cin: 4;" "uint8 cn: 4;" bitfield definitions? At least this is better than "unsigned" only? This would then be an endianness bug? Can you show where BITFIELD macros are used in Haiku code?

I will remove those stray comments (copied from USB_audio.h).

[mmlr]

Replying to Karvjorm:

I noticed that all those header files (USB_audio.h, USB_hid.h, USB_midi.h) use anonymous structs so I have used the same model.

Well no problem then. Maybe our gcc2 can handle them by now or I just remember wrong, but I thought there were issues with anonymous unions at least.

Also, in USB_midi.h there are "uint8 cin: 4;" "uint8 cn: 4;" bitfield definitions? At least this is better than "unsigned" only? This would then be an endianness bug? Can you show where BITFIELD macros are used in Haiku code?

Yes, at least use uint32 for example. The bitfield macros take the endianness into account by switching the delaration order depending on the target platform, see source:/haiku/trunk/headers/private/drivers/lendian_bitfield.h for the definitions and source:/haiku/trunk/src/add-ons/kernel/bus_managers/ide/ide_device_infoblock.h for a usage example. That file should probably be made public anyway.

[Karvjorm]

I have updated USB_video.h file but it is not quite complete yet. There are some tables that I don't quite understand and I have to read the whole source document again some times.

Anyway, I wonder if there is any idea to document structs like status_packet_format struct I have already done? Those that want to use this header file and implement usb video add-ons have to read source document anyway.

[Karvjorm]

An updated USB_video.h file

[mmadia]

This will tell you how to make the patch : ​http://dev.haiku-os.org/wiki/SubmittingPatches#Preparingtocreatepatchfile

basically,

svn add USB_video.h
cd into haiku top directory
svn diff > usb_video.patch

[Karvjorm]

Replying to mmadia:

This will tell you how to make the patch : ​http://dev.haiku-os.org/wiki/SubmittingPatches#Preparingtocreatepatchfile

basically,

svn add USB_video.h
cd into haiku top directory
svn diff > usb_video.patch

Thanks, I did not understand, that I can add new files to svn tree via patching. My header file is not ready yet, but maybe I can add it already now and update it later. It was available here only for the comments.

[Karvjorm]

A patch to add USB_video.h to svn tree

[mmlr]

Replying to Karvjorm:

My header file is not ready yet, but maybe I can add it already now and update it later.

Hi there. The header looks basically fine by now, what do you mean by "not ready yet"? The only thing I don't like is that the structs aren't declared with the same name as they are typedef'ed to, meaning two names polluting the namespace (i.e. typedef struct struct_name {} usb_video_struct_name). Also the work on the UVC webcam add-on has sprung a USB_video.h of its own in src/add-ons/media/media-add-ons/usb_webcam/addons/uvc/USB_video.h which looks like it (obviously) defines some of the same values. I'd prefer to use the header attached here for its completeness and have the other one removed and dependent code "ported" to this header. For now the attached header could be commited as soon as the struct naming thing is clear.

[Karvjorm]

Hi, if I can recall it correct, there were couple of tables in the USB Video specification that I did not quite understand and therefore specification is not 100 percent completed. I planned to return to these tables later, but forgot. But you can well use the present header file now, if you want. Also, you can change typedefs so that there is only one struct name in the namespace.

Best regards,

Karvjorm

[modeenf]

Any news about this?

[korli]

(In #9933) I removed the relation with UVC and isochronous streams as these devices use a vendor specific bulk-only interface.

[Karvjorm]

This has been on my TODO list, but I hardly have time for this. Please, edit and use it as you will.

[pulkomandy]

I did not notice about this ticket back then and implemented USB_video.h on my own in hrev50651 (along with support in listusb).

However, this version has more enumerations, which would be nice to have in the header file. It would be great if someone could merge tthe two files. Marking the ticket as easy, because that should not be too hard to do!

[iambrj]

Are the typedefs also supposed to be merged?

[pulkomandy]

Yes, if some values are missing from the current file.

[iambrj]

merged enums and typedefs from both the USB_video.h

[iambrj]

I was able to merge both the files (enums and typedefs) into USB_video.2.h (attachment)

[pulkomandy]

Hi,

Can you upload the changes to Gerrit at review.haiku-os.org?

See https://dev.haiku-os.org/wiki/CodingGuidelines/SubmittingPatches

[iambrj]

Uploaded to Gerrit ​https://review.haiku-os.org/c/haiku/+/1022

[iambrj]

fixed all the mistakes mentioned in code review 1022

[pulkomandy]

Please use the code review system. I even included comments in my review explaining what to do to update youre review request. There is no need to attach the files here.

[iambrj]

Replying to pulkomandy:

Please use the code review system. I even included comments in my review explaining what to do to update youre review request. There is no need to attach the files here.

I added it here because I was unable to log into my account because of the "Forbidden" page. However, I have created a new Github and a new account on review.haiku-os.org

Also, I've fixed all the issues mentioned in the code review.

Thank you for being so patient and helping me correct all my silly mistakes

​https://review.haiku-os.org/c/haiku/+/1024

[pulkomandy]

​https://review.haiku-os.org/c/haiku/+/1022 has been merged.

[nielx]

Assign tickets with status=closed and resolution=fixed within the R1/beta2 development window to the R1/beta2 Milestone