Opened 10 years ago

Last modified 10 months ago

#5940 reopened enhancement

Header file for the USB video driver (easy)

Reported by: Karvjorm Owned by: mmlr
Priority: normal Milestone: R1
Component: Drivers/USB Version: R1/Development
Keywords: USB video drivers Cc: Karvjorm
Blocked By: Blocking:
Has a Patch: no Platform: All

Description

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

Attachments (4)

USB_video.h (20.9 KB ) - added by Karvjorm 10 years ago.
An updated USB_video.h file
USB_video.patch (22.8 KB ) - added by Karvjorm 9 years ago.
A patch to add USB_video.h to svn tree
USB_video.2.h (21.7 KB ) - added by iambrj 10 months ago.
merged enums and typedefs from both the USB_video.h
USB_video.3.h (21.6 KB ) - added by iambrj 10 months ago.
fixed all the mistakes mentioned in code review 1022

Download all attachments as: .zip

Change History (27)

comment:1 by mmlr, 10 years ago

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.

comment:2 by Karvjorm, 10 years ago

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).

in reply to:  2 comment:3 by mmlr, 10 years ago

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.

comment:4 by Karvjorm, 10 years ago

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.

by Karvjorm, 10 years ago

Attachment: USB_video.h added

An updated USB_video.h file

comment:5 by mmadia, 9 years ago

Has a Patch: set

comment:6 by mmadia, 9 years ago

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

in reply to:  6 ; comment:7 by Karvjorm, 9 years ago

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.

by Karvjorm, 9 years ago

Attachment: USB_video.patch added

A patch to add USB_video.h to svn tree

in reply to:  7 comment:8 by mmlr, 8 years ago

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.

comment:9 by mmlr, 8 years ago

Has a Patch: unset

comment:10 by Karvjorm, 8 years ago

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

comment:11 by modeenf, 6 years ago

Blocking: 9933 added

comment:12 by modeenf, 6 years ago

Any news about this?

comment:13 by korli, 6 years ago

Blocking: 9933 removed

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

comment:14 by Karvjorm, 6 years ago

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

comment:15 by pulkomandy, 11 months ago

Resolution: fixed
Status: newclosed
Summary: Header file for the USB video driverHeader file for the USB video driver (easy)

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!

comment:16 by pulkomandy, 11 months ago

Resolution: fixed
Status: closedreopened

comment:17 by iambrj, 10 months ago

Are the typedefs also supposed to be merged?

comment:18 by pulkomandy, 10 months ago

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

by iambrj, 10 months ago

Attachment: USB_video.2.h added

merged enums and typedefs from both the USB_video.h

comment:19 by iambrj, 10 months ago

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

comment:20 by pulkomandy, 10 months ago

Hi,

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

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

by iambrj, 10 months ago

Attachment: USB_video.3.h added

fixed all the mistakes mentioned in code review 1022

comment:22 by pulkomandy, 10 months ago

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.

in reply to:  22 comment:23 by iambrj, 10 months ago

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

Note: See TracTickets for help on using tickets.