Aquileo | rs usb api and implementations added by matkatz · Pull Request #3828 · realsenseai/librealsense

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


-        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

        }

        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


        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

+                    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

-                    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

+            {
+                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

+
+            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

+                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

+                    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

+        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

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


Could be made const

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

+            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

+
+        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

+            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

        }

-        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

            {
                auto dev = _source->create_usb_device(info);
-
+                


trailing WS

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

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


Indentation

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

+            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

+            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

+                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

+
+            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

+
+    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 dorodnic and ev-mp 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 suggested 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 realsenseai: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();

Conversation

matkatz commented Apr 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matkatz May 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ev-mp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

matkatz commented Apr 23, 2019 •
edited

Loading

matkatz May 3, 2019 •
edited

Loading