rs usb api and implementations added #3828

matkatz · 2019-04-23T19:12:47Z

This PR creates a common USB API for the different operating systems.
As first step this API will be used for cross platform FW update API and tools.
In the future this infrastructure will allow single implementation for UVC/HID/TM2 for all the supported operating systems.

DSO-10947

ev-mp · 2019-05-01T20:18:35Z

src/android/android-backend.cpp

        }

-        std::shared_ptr<usb_device> android_backend::create_usb_device(usb_device_info info) const {
-            throw std::runtime_error("create_usb_device Not supported");
+        std::shared_ptr<command_transfer> android_backend::create_usb_device(usb_device_info info) const {


The method create_usb_device returns command_transfer ?
If so - please use "_dev" suffix as command_transfer is too vague
Also is this intended to work with non-Realsense devices, such as platform cam ?

the original usb_device was an empty class which inherited command_transfer, since I removed usb_device I needed to replace it with command_transfer but I don't want to make more changes to the backend API in this PR unless it is necessary .

ev-mp · 2019-05-01T20:19:23Z

src/android/android-backend.cpp

        }

        std::vector<usb_device_info> android_backend::query_usb_devices() const {
+            std::vector<usb_device_info> results;


Pls change the name to query_usb_devices_**info** for consistency

ev-mp · 2019-05-01T20:23:00Z

src/android/android-backend.cpp

@@ -68,7 +80,7 @@ namespace librealsense {


        std::shared_ptr<device_watcher> android_backend::create_device_watcher() const {
-            return std::make_shared<usb_host::device_watcher>();
+            return device_watcher_usbhost::instance();


There should be no need to wrap a singleton with shared_ptr.

I need to implement the backend API:
std::shared_ptr<device_watcher> create_device_watcher() const override;

ev-mp · 2019-05-01T20:24:56Z

src/android/android-uvc.cpp

+                    uvc->vid = dev->get_info().vid;
+                    uvc->pid = dev->get_info().pid;
+                    usbhost_open(uvc.get(), mi);
+                    return uvc;


This function will open the first device with matched MI. How can one query the 2nd or 3rd device ?

ev-mp · 2019-05-01T20:25:37Z

src/android/android-uvc.cpp

-                    if (device->deviceData.ctrl_if.bInterfaceNumber == mi) {
-                        _device = device;
+            if (state == D0 && _power_state == D3) {
+                uint16_t vid = _info.vid, pid = _info.pid, mi = _info.mi;


vid/pid seem unused in this segment - pls recheck

will remove

ev-mp · 2019-05-01T21:20:05Z

src/libusb/device-libusb.cpp

+            {
+                auto i = entry.second;
+                if(filter == USB_SUBCLASS_ANY ||
+                    i->get_subclass() & filter ||


Put parentheses

function will be removed

ev-mp · 2019-05-01T21:22:13Z

src/libusb/device-libusb.h

+
+            virtual const usb_device_info get_info() const override { return _infos[0]; }
+            virtual const std::vector<usb_device_info> get_subdevices_infos() const override { return _infos; }
+            virtual const rs_usb_interface get_interface(uint8_t interface_number) const override { return _interfaces.at(interface_number); }


This will throw if the index is not in the map

get_interface(uint8_t interface_number) will be removed, leaving only get_interfaces()

ev-mp · 2019-05-01T21:29:06Z

src/libusb/enumerator-libusb.cpp

+                libusb_device_descriptor desc = { 0 };
+
+                auto rc = libusb_get_device_descriptor(lusb_device, &desc);
+                if (desc.idVendor != 0x8086)


For review - probably requires some sort of a mask - T265 , etc'
Log the devices that are filtered out

devices are filtered on context.cpp via pick__devices

I have also removed the 0x8086 filter

ev-mp · 2019-05-01T21:31:08Z

src/libusb/enumerator-libusb.cpp

+                    break;
+                }
+            }
+            libusb_free_device_list(list, 0);


A common pattern in this and the next functions:
libusb_get_device_list
...
libusb_free_device_list

Needs a common RAII support

will make RAII

ev-mp · 2019-05-01T21:32:43Z

src/libusb/handle-libusb.h

+        class handle_libusb
+        {
+        public:
+            handle_libusb(libusb_device* device, uint8_t interface, int timeout_ms = 100) :


Add a comment how this value was selected

timeout will be removed from the handle signature

ev-mp · 2019-05-02T05:43:02Z

src/libusb/messenger-libusb.cpp

+
+        }
+
+        bool usb_messenger_libusb::reset_endpoint(std::shared_ptr<usb_endpoint> endpoint)


Could be made const

ev-mp · 2019-05-02T05:46:08Z

src/libusb/messenger-libusb.cpp

+            if(rv)
+                LOG_DEBUG("USB pipe " << ep << " reset successfully");
+            else
+                LOG_DEBUG("Failed to reset the USB pipe " << ep);


The actual result of the transfer is not registered (reduced to bool) will make it harder for debugging and maintenance

usb status added to the log

ev-mp

A huge transformation.
We'll need multiple things to review and polish.
The one thing - the class diagram should clearly depict the inheritance tree with all the interfaces and concrete classes. It is very hard to navigate otherwise

ev-mp · 2019-05-02T05:49:49Z

src/libusb/messenger-libusb.cpp

+
+        int usb_messenger_libusb::control_transfer(int request_type, int request, int value, int index, uint8_t* buffer, uint32_t length, uint32_t timeout_ms)
+        {
+            handle_libusb handle(_device->get_device(), index & 0xFF, timeout_ms);


Please either perform a static_cast or add a note that the actual index is within 0-255.
You also need to check the input against things such as 0xabcd0011, which could be interpret as 0x11 without checking the higher bits

the upper bytes are required for control transfer, the mask is extracting the interface number

ev-mp · 2019-05-02T05:52:12Z

src/libusb/messenger-libusb.cpp

+            return rv;
+        }
+
+        int usb_messenger_libusb::bulk_transfer(const std::shared_ptr<usb_endpoint>& endpoint, uint8_t* buffer, uint32_t length, uint32_t timeout_ms)


PLs make the in and out length param type identical (preferably int)

returned value replaced with usb status

ev-mp · 2019-05-02T05:56:42Z

src/mf/mf-backend.cpp

@@ -66,35 +65,28 @@ namespace librealsense
            return devices;
        }

-        std::shared_ptr<usb_device> wmf_backend::create_usb_device(usb_device_info info) const
+        std::shared_ptr<command_transfer> wmf_backend::create_usb_device(usb_device_info info) const


Make the param const&

I prefer to avoid changes to the backend API at this point

ev-mp · 2019-05-02T05:57:27Z

src/mock/recorder.cpp

        {
            return try_record([&](recording* rec, lookup_key k)
            {
                auto dev = _source->create_usb_device(info);
-
+                


trailing WS

ev-mp · 2019-05-02T05:58:05Z

src/mock/recorder.cpp

@@ -1136,7 +1136,7 @@ namespace librealsense
            return _rec->load_uvc_device_info_list();


Indentation

ev-mp · 2019-05-02T06:32:14Z

src/winusb/device-winusb.cpp

+            for (auto&& entry : _interfaces)
+            {
+                auto i = entry.second;
+                if(filter == USB_SUBCLASS_ANY || 


This code is multiplied several times (counted 3) - can it be refactored?

it will be removed

ev-mp · 2019-05-02T06:32:41Z

src/winusb/endpoint-winusb.h

+            virtual endpoint_type get_type() const override { return _type; }
+            virtual endpoint_direction get_direction() const override
+            { 
+                return _address >= USB_ENDPOINT_DIRECTION_READ ?


This code is also C&P

ev-mp · 2019-05-02T06:34:43Z

src/winusb/enumerator-winusb.cpp

+                return false;
+            for (auto&& guid : guids)
+            {
+                for (auto&& id : query_by_interface(guid, L"8086"))


The 8086 mask may not recognize T265

will remove the filter

ev-mp · 2019-05-02T06:35:08Z

src/winusb/enumerator-winusb.cpp

+
+            for (auto&& guid : guids)
+            {
+                for (auto&& id : query_by_interface(guid, L"8086"))


Same as above

same as above

ev-mp · 2019-05-02T06:37:55Z

unit-tests/internal/internal-tests-usb.cpp

+
+    switch (devices.size())
+    {
+        case 0: printf("\nno USB device found\n\n"); break;


This case will not be invoked - terminated on previous REQUIRE(devices.size());

matkatz requested review from ev-mp and dorodnic April 23, 2019 19:12

rsusb api, implementation and tests added

d19b987

matkatz force-pushed the rsusb-api branch from 8c46fe7 to d19b987 Compare May 1, 2019 15:10

ev-mp reviewed May 1, 2019

View reviewed changes

ev-mp reviewed May 2, 2019

View reviewed changes

ev-mp requested changes May 2, 2019

View reviewed changes

matkatz added 2 commits May 8, 2019 10:49

rsusb - code review fix

a0d7362

rsusb libusb - avoid releasing interface if open failed

880b097

ev-mp approved these changes May 16, 2019

View reviewed changes

ev-mp merged commit f51b203 into IntelRealSense:development May 16, 2019

matkatz deleted the rsusb-api branch July 9, 2019 12:51


		}

		bool usb_messenger_libusb::reset_endpoint(std::shared_ptr<usb_endpoint> endpoint)

		@@ -1136,7 +1136,7 @@ namespace librealsense
		return _rec->load_uvc_device_info_list();

rs usb api and implementations added #3828

rs usb api and implementations added #3828

Conversation

matkatz commented Apr 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matkatz May 3, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ev-mp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matkatz commented Apr 23, 2019 •

edited

Loading

matkatz May 3, 2019 •

edited

Loading