Hi Alex,
> here's the patch that adds support for USB transport layer via libusb to
> openobex library. The patch for the obex_test application is in the next
> message. Specification for these USB OBEX interfaces is available at
> http://www.usb.org/developers/devclass_docs
>
> Devices which support this are typically post-2003 mobile phones with a
> USB interface. The list of contributors to the specification includes
> Ericsson, Motorola, NEC, Nokia, NTT DoCoMo and others. The only devices
> which are known to work so far are my Nokia 7610 and 6630 phones, and of
> course I'd like to hear from you about other gadgets.
>
> Please note that many phones which claim to be obex-compatible don't
> actually have a proper OBEX interface: you switch their ACM modems to OBEX
> mode instead with a special AT command. For Nokia phones the rule of thumb
> is this: if it comes with a DKU-2 cable, it has a proper OBEX interface,
> and should work with this patch. If it comes with a DKU-5 or any other
> cable, then it's using some other method and isn't compatible. The
> compatibility chart is here:
> http://www.nokia.com/nokia/0,,54698,00.html#apps
>
> I have also added support for USB to obexftp library and application, but
> that's coming later; it works, but it's not ready for others to see yet.
>
> To use this, you first run "obex_test -u" to get a list of USB OBEX
> interfaces on your system. Then "obex_test -u interface_number" to connect
> to one of them.
>
> So I'd like to get your comments on the patch, and of course ultimately
> have it included into openobex.
thanks for the patch, but I am not sure when I will find the time to
review it completly. However some comments and please don't diff any
object files.
> diff -uNr -x configure -x aclocal.m4 -x autom4te.cache -x config.h -x
> config.h.in -x config.log -x config.status -x depcomp -x doc -x INSTALL -x
> install-sh -x libtool -x m4macros -x Makefile -x Makefile.in -x missing -x
> mkinstalldirs -x '*.lo' -x '*.Plo' -x '*.la*' -x .libs -x stamp-h1
> openobex-1.0.1/src/obex.c openobex-1.0.1-usb/src/obex.c
> --- openobex-1.0.1/src/obex.c 2003-10-01 14:17:13.000000000 +0300
> +++ openobex-1.0.1-usb/src/obex.c 2005-02-10 01:29:18.000000000 +0200
> @@ -50,11 +50,15 @@
> #include "obex_object.h"
> #include "obex_connect.h"
> #include "obex_client.h"
> +#include "obex_const.h"
why is this now needed?
> @@ -121,25 +126,39 @@
> /* Init transport */
> self->trans.type = transport;
> self->trans.connected = FALSE;
> +
> + self->usb_interfaces = NULL;
> + if ( transport == OBEX_TRANS_USB ) {
check your coding style. It should be "if (transport == ..) {". There
are more of them. Fix them all.
> +#ifdef HAVE_USB
> + self->usb_interfaces = usbobex_find_interfaces();
> +#endif
Doing
#else
self->usb_interfaces = NULL;
#endif
is better then double assignment. However I still don't understand why
self->usb_interfaces is needed.
> + /* Safe values.
> + * Both self->mtu_rx and self->mtu_tx_max can be increased by app
> + * self->mtu_tx will be whatever the other end sneds us - Jean II */
Against what version do you diff. You are not Jean. Do the diff against
the CVS version and if you wanna change basic OpenOBEX send in a extra
patch for it.
> @@ -182,16 +201,26 @@
> */
> void OBEX_Cleanup(obex_t *self)
> {
> + struct usb_obex_intf* next;
> +
> obex_return_if_fail(self != NULL);
>
> obex_transport_disconnect_request(self);
> obex_transport_disconnect_server(self);
> -
> +
> if (self->tx_msg)
> g_netbuf_free(self->tx_msg);
>
> if (self->rx_msg)
> g_netbuf_free(self->rx_msg);
> +
> + if ( self->trans.type == OBEX_TRANS_USB ) {
> + while (self->usb_interfaces != NULL) {
> + next = self->usb_interfaces->next;
> + free(self->usb_interfaces);
> + self->usb_interfaces = next;
> + }
> + }
And again. I need a reason for it. This is generic code and I don't see
any need to touch it.
Even if it is needed, if USB support is not compiled in there should be
no code path for it.
> +
> + obex_return_val_if_fail(self != NULL, -1);
> + obex_return_val_if_fail(interface != NULL, -1);
> + obex_return_val_if_fail(interface->device != NULL, -1);
The coding style must be wrong. We use tabs.
> diff -uNr -x configure -x aclocal.m4 -x autom4te.cache -x config.h -x
> config.h.in -x config.log -x config.status -x depcomp -x doc -x INSTALL -x
> install-sh -x libtool -x m4macros -x Makefile -x Makefile.in -x missing -x
> mkinstalldirs -x '*.lo' -x '*.Plo' -x '*.la*' -x .libs -x stamp-h1
> openobex-1.0.1/src/obex.def openobex-1.0.1-usb/src/obex.def
> --- openobex-1.0.1/src/obex.def 2003-10-01 14:17:13.000000000 +0300
> +++ openobex-1.0.1-usb/src/obex.def 2005-02-07 01:05:06.000000000 +0200
> @@ -10,6 +10,7 @@
> OBEX_GetFD
> OBEX_HandleInput
> OBEX_ServerRegister
> +OBEX_ServerAccept
> OBEX_Request
> OBEX_CancelRequest
> OBEX_SetUserData
This is not part of the USB patch. Send a separate one to fix it.
> @@ -9,6 +9,7 @@
> OBEX_GetFD
> OBEX_HandleInput
> OBEX_ServerRegister
> +OBEX_ServerAccept
> OBEX_Request
> OBEX_CancelRequest
> OBEX_SetUserData
See above.
> diff -uNr -x configure -x aclocal.m4 -x autom4te.cache -x config.h -x
> config.h.in -x config.log -x config.status -x depcomp -x doc -x INSTALL -x
> install-sh -x libtool -x m4macros -x Makefile -x Makefile.in -x missing -x
> mkinstalldirs -x '*.lo' -x '*.Plo' -x '*.la*' -x .libs -x stamp-h1
> openobex-1.0.1/src/obex_const.h openobex-1.0.1-usb/src/obex_const.h
> --- openobex-1.0.1/src/obex_const.h 2003-10-01 14:17:13.000000000 +0300
> +++ openobex-1.0.1-usb/src/obex_const.h 2005-02-10 01:50:15.000000000
> +0200
> @@ -47,6 +47,31 @@
> void * customdata;
> } obex_ctrans_t;
>
> +struct usb_obex_intf;
> +
> +/* Structure that contains information about a USB OBEX interface
> + * present on the system */
> +struct usb_obex_intf {
> + struct usb_obex_intf *prev, *next; /* Next and previous interfaces
> in the list */
> + struct usb_device *device; /* USB device that has the
> interface */
> + int configuration; /* Device configuration */
> + int control_interface; /* OBEX master interface */
> + int control_interface_description; /* OBEX master interface string
> descriptor number
> + * If non-zero, use
> usb_get_string_simple() from
> + * libusb to retrieve
> human-readable description
> + */
> + int control_setting; /* OBEX master interface
> setting */
> + int data_interface; /* OBEX data/slave interface */
> + int data_idle_setting; /* OBEX data/slave idle setting
> */
> + int data_interface_idle_description; /* OBEX data/slave interface
> string descriptor number
> + * in idle setting */
> + int data_active_setting; /* OBEX data/slave active
> setting */
> + int data_interface_active_description; /* OBEX data/slave interface
> string descriptor number
> + * in active setting */
> + int data_endpoint_read; /* OBEX data/slave interface
> read endpoint */
> + int data_endpoint_write; /* OBEX data/slave interface
> write endpoint */
> +};
> +
This can't be part of the public interface. Hide it inside the USB
specific transport layer. What happens when the library is compiled
without USB support?
> @@ -133,6 +159,7 @@
> #define OBEX_DEFAULT_MTU 1024
> #define OBEX_MINIMUM_MTU 255
> #define OBEX_MAXIMUM_MTU 32768
> +#define OBEX_DEFAULT_MTU_USB 0xffff
Why?
> diff -uNr -x configure -x aclocal.m4 -x autom4te.cache -x config.h -x
> config.h.in -x config.log -x config.status -x depcomp -x doc -x INSTALL -x
> install-sh -x libtool -x m4macros -x Makefile -x Makefile.in -x missing -x
> mkinstalldirs -x '*.lo' -x '*.Plo' -x '*.la*' -x .libs -x stamp-h1
> openobex-1.0.1/src/obex_main.h openobex-1.0.1-usb/src/obex_main.h
> --- openobex-1.0.1/src/obex_main.h 2003-10-01 14:17:13.000000000 +0300
> +++ openobex-1.0.1-usb/src/obex_main.h 2005-02-06 23:07:52.000000000
> +0200
> @@ -139,6 +139,9 @@
>
> obex_transport_t trans; /* Transport being used */
> obex_ctrans_t ctrans;
> +
> + struct usb_obex_intf* usb_interfaces; /* Used by USB OBEX transport*/
> +
> void * userdata; /* For user */
> };
Explain why this is needed?
> default:
> DEBUG(4, "Transport not implemented!\n");
> @@ -389,6 +406,16 @@
> case OBEX_TRANS_FD:
> actual = do_write(self->writefd, msg, self->trans.mtu);
> break;
> +#ifdef HAVE_USB
> + case OBEX_TRANS_USB:
> + if ( self->trans.connected != TRUE )
> + break;
> + DEBUG(4, "Endpoint %d\n",
> self->trans.self.usb.interface->data_endpoint_write);
> + actual = usb_bulk_write(self->trans.self.usb.dev_data, \
> + self->trans.self.usb.interface->data_endpoint_write, \
> + msg->data, msg->len, 10*1000);
For what do you need the "\"?
> +#ifdef HAVE_USB
> + case OBEX_TRANS_USB:
> + if ( self->trans.connected != TRUE )
> + break;
> + DEBUG(4, "Endpoint %d\n",
> self->trans.self.usb.interface->data_endpoint_read);
> + actual = usb_bulk_read(self->trans.self.usb.dev_data, \
> + self->trans.self.usb.interface->data_endpoint_read, \
> + msg->tail, self->mtu_rx, 10*1000);
> + break;
See above.
> +#ifdef HAVE_USB
> +#include "usb_wrap.h"
> +#endif /*HAVE_USB*/
And what is "usb_wrap.h"?
> @@ -76,7 +82,7 @@
> int obex_transport_read(obex_t *self, int count, uint8_t *buf, int buflen);
>
>
> -#endif OBEX_TRANSPORT_H
> +#endif //OBEX_TRANSPORT_H
Again, against what version do you diff? If this is a core bug, then in
a patch for it.
> Binary files openobex-1.0.1/src/obex_transport.o and
> openobex-1.0.1-usb/src/obex_transport.o differ
> diff -uNr -x configure -x aclocal.m4 -x autom4te.cache -x config.h -x
> config.h.in -x config.log -x config.status -x depcomp -x doc -x INSTALL -x
> install-sh -x libtool -x m4macros -x Makefile -x Makefile.in -x missing -x
> mkinstalldirs -x '*.lo' -x '*.Plo' -x '*.la*' -x .libs -x stamp-h1
> openobex-1.0.1/src/usb_wrap.h openobex-1.0.1-usb/src/usb_wrap.h
> --- openobex-1.0.1/src/usb_wrap.h 1970-01-01 02:00:00.000000000 +0200
> +++ openobex-1.0.1-usb/src/usb_wrap.h 2005-01-24 03:46:38.000000000 +0200
> @@ -0,0 +1,12 @@
> +#ifndef USB_WRAP_H
> +#define USB_WRAP_H
> +
> +#include <usb.h>
> +
> +struct addr_usb {
> + struct usb_obex_intf* interface;
> + usb_dev_handle* dev_control;
> + usb_dev_handle* dev_data;
> +};
> +
> +#endif /* USB_WRAP_H */
Ah, this is "usb_wrap.h" and why do you need it?
> +struct usb_obex_intf* usbobex_find_interfaces()
> +{
> + struct usb_bus *busses;
> + struct usb_obex_intf *current, *next;
> + current = NULL; next = NULL;
> +
> + usb_init();
> + usb_find_busses();
> + usb_find_devices();
> +
> + busses = usb_get_busses();
> +
> + struct usb_bus *bus;
> + int c, i, a;
May you wanna try a GCC 2.95 with this part of code. Do it right. We
declare variables at the top of a function or a the top of a block.
> + for (bus = busses; bus; bus = bus->next) {
> + struct usb_device *dev;
> +
> + for (dev = bus->devices; dev; dev = dev->next) {
> + /* Loop through all of the configurations */
> + for (c = 0; c < dev->descriptor.bNumConfigurations;
> c++) {
> + /* Loop through all of the interfaces */
> + for (i = 0; i < dev->config[c].bNumInterfaces;
> i++) {
> + /* Loop through all of the alternate
> settings */
> + for (a = 0; a <
> dev->config[c].interface[i].num_altsetting; a++) {
> + /* Check if this interface is
> OBEX */
> + if
> ((dev->config[c].interface[i].altsetting[a].bInterfaceClass == USB_CDC_CLASS)
> + &&
> (dev->config[c].interface[i].altsetting[a].bInterfaceSubClass ==
> USB_CDC_OBEX_SUBCLASS)) {
> + /* Find the data
> interface */
> +
> + unsigned char *buffer =
> dev->config[c].interface[i].altsetting[a].extra;
> + int buflen =
> dev->config[c].interface[i].altsetting[a].extralen;
> +
> + next =
> malloc(sizeof(struct usb_obex_intf));
> + if (next == NULL)
> + continue;
> + next->device = dev;
> + next->configuration
> = dev->config[c].bConfigurationValue;
> +
> next->control_interface =
> dev->config[c].interface[i].altsetting[a].bInterfaceNumber;
> +
> next->control_interface_description =
> dev->config[c].interface[i].altsetting[a].iInterface;
> +
> next->control_setting =
> dev->config[c].interface[i].altsetting[a].bAlternateSetting;
> +
> + int err =
> find_obex_data_interface(buffer, buflen, dev->config[c], next);
> + if (err)
> + free(next);
> + else {
> + if (current)
> +
> current->next = next;
> + next->prev =
> current;
> + next->next =
> NULL;
> + current = next;
> + }
> + }
> + }
> + }
> + }
> + }
> + }
Come on. Break this up into smaller part. Nobody can read or understand
it. You reached the end of a 80 width terminal.
> + switch (buffer [2]) {
Coding style mistake. This must be "switch (buffer[2]) {". Follow it.
> + /* Found the slave interface, now find active/idle settings and
> endpoints */
> + intf->data_interface = union_header->bSlaveInterface0;
> + int c, i, a;
Test this with a GCC 2.95. See point above.
> + struct usb_endpoint_descriptor *ep0, *ep1;
> + int found_active = 0;
> + int found_idle = 0;
> + struct usb_bus* bus;
> +
> + /* Loop through all of the interfaces */
> + for (i = 0; i < config.bNumInterfaces; i++) {
> + /* Loop through all of the alternate settings */
> + for (a = 0; a < config.interface[i].num_altsetting; a++) {
> + /* Check if this interface is OBEX data interface*/
> + if (config.interface[i].altsetting[a].bInterfaceNumber
> == intf->data_interface) {
> + if
> (config.interface[i].altsetting[a].bNumEndpoints == 2) {
> + ep0 =
> config.interface[i].altsetting[a].endpoint;
> + ep1 =
> config.interface[i].altsetting[a].endpoint + 1;
> + if ((ep0->bEndpointAddress &
> USB_ENDPOINT_IN) && ((ep0->bmAttributes & USB_ENDPOINT_TYPE_MASK) ==
> USB_ENDPOINT_TYPE_BULK) && !(ep1->bEndpointAddress & USB_ENDPOINT_IN) &&
> ((ep1->bmAttributes & USB_ENDPOINT_TYPE_MASK) == USB_ENDPOINT_TYPE_BULK)) {
> + found_active = 1;
> + intf->data_active_setting =
> config.interface[i].altsetting[a].bAlternateSetting;
> +
> intf->data_interface_active_description =
> config.interface[i].altsetting[a].iInterface;
> + intf->data_endpoint_read =
> ep0->bEndpointAddress;
> + intf->data_endpoint_write =
> ep1->bEndpointAddress;
> + }
> + if (!(ep0->bEndpointAddress &
> USB_ENDPOINT_IN) &&((ep0->bmAttributes & USB_ENDPOINT_TYPE_MASK) ==
> USB_ENDPOINT_TYPE_BULK) && (ep1->bEndpointAddress & USB_ENDPOINT_IN)
> &&((ep1->bmAttributes & USB_ENDPOINT_TYPE_MASK) == USB_ENDPOINT_TYPE_BULK)) {
> + found_active = 1;
> + intf->data_active_setting =
> config.interface[i].altsetting[a].bAlternateSetting;
> +
> intf->data_interface_active_description =
> config.interface[i].altsetting[a].iInterface;
> + intf->data_endpoint_read =
> ep1->bEndpointAddress;
> + intf->data_endpoint_write =
> ep0->bEndpointAddress;
> + }
> + }
> + if
> (config.interface[i].altsetting[a].bNumEndpoints == 0) {
> + found_idle = 1;
> + intf->data_idle_setting =
> config.interface[i].altsetting[a].bAlternateSetting;
> + intf->data_interface_idle_description =
> config.interface[i].altsetting[a].iInterface;
> + }
> + }
> + }
> + }
Break this loop up. It is too nested.
> + if(ret < 0) {
Coding style mistake. This should be "if (ret < 0) {". Fix them all.
> +
> + ret = usb_set_configuration(self->trans.self.usb.dev_control,
> self->trans.self.usb.interface->configuration);
> + if(ret < 0) {
This is a tab versus spaces problem. Fix all of them.
> +/* class and subclass specific descriptor types */
> +#define CDC_HEADER_TYPE 0x00
> +#define CDC_CALL_MANAGEMENT_TYPE 0x01
> +#define CDC_AC_MANAGEMENT_TYPE 0x02
> +#define CDC_UNION_TYPE 0x06
> +#define CDC_COUNTRY_TYPE 0x07
> +#define CDC_OBEX_TYPE 0x15
> +
> +/* Interface descriptor */
> +#define USB_DT_CS_INTERFACE 0x24
> +#define CDC_DATA_INTERFACE_TYPE 0x0a
Looks like tab versus spaces again.
> Binary files openobex-1.0.1/src/usbobex.o and
> openobex-1.0.1-usb/src/usbobex.o differ
You better exclude *.o files from the diff ;)
This was the first look through your code. You should really make sure
that everything is still working when USB support is not compiled in. I
think it is a good idea to break your patch up a little bit. One patch
that adds usbobex.[ch] and the other one for the needed changes in the
main files.
Regards
Marcel
-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
