pbio/drv/usb: Add common driver. #410
Conversation
This will help prepare the common USB driver code, and test USB and BLE both enabled.
This de-duplicates common USB code such as handling stdout or sending the hub status. This was quite hard to keep track of between the EV3 and STM32 drivers, causing subtle differences and issues which were not so easy to synchronize. The simulation driver shows the bare minimum driver-specific functions that each driver must implement, which will we do in the next commits.
The platform specific drivers have specific requirements and cleanups for the outgoing data, and they have different buffers for each. So get those buffers instead of using a local one.
This applies the newly added common driver code to EV3 and STM32, stripping away most of the code that is the same among them. Hooks have been added so that the NXT driver builds, but it is not functional yet.
| #define PBDRV_CONFIG_UART_EV3_NUM_UART (5) | ||
|
|
||
| #define PBDRV_CONFIG_USB (1) | ||
| #define PBDRV_CONFIG_USB_MAX_PACKET_SIZE (512) |
There was a problem hiding this comment.
This doesn't seem like the right place to set this value. It depends on the specific driver implementation. And we already specify the driver here, e.g. with PBDRV_CONFIG_USB_EV3
Instead, I would make a per-driver header and do something like:
#if PBDRV_CONFIG_USB_EV3
#import "usb_ev3_params.h"
#if PBDRV_CONFIG_USB_STM32F4
#import "usb_stm32_params.h"
#elif
...Where each header defines e.g. PBDRV_USB_MAX_PACKET_SIZE.
There was a problem hiding this comment.
This sounds like a lot of extra files and ifdefs compared to a single constant. We do use this pattern in a few other places. But it might not matter if we move this to sys anyway.
There was a problem hiding this comment.
I'm sure there are other more elegant solutions as well. 😄
lib/pbio/drv/usb/usb.c
Outdated
| switch (data_in[0]) { | ||
| case PBIO_PYBRICKS_OUT_EP_MSG_SUBSCRIBE: | ||
| pbdrv_usb_events_subscribed = data_in[1]; | ||
| respond_result = PBIO_PYBRICKS_ERROR_OK; |
There was a problem hiding this comment.
Having global variables without a namespace prefix really throws me off. It makes it looks like this value is assigned but never used. You have go read the entire function up to this point to see that it isn't a local variable, whereas if we follow the convention of namespace prefix on all global variable, it is instantly obvious what is going on.
There was a problem hiding this comment.
Happy to change, but FWIW we've been using prefixes consistently for global public variables shared across files, but less so for static variables within files. I find it helpful to tell these apart.
There was a problem hiding this comment.
It doesn't look like there is anything hardware-specific here, it would make more sense to me to have this new layer be pbsys instead of pbdrv. (Kind of like BlueZ manages the Bluetooth stack on Linux at the OS level doing most of it in userspace and only calls into the kernel to actually poke the hardware).
There was a problem hiding this comment.
Yeah, the same probably goes for the newly structured Bluetooth drivers. It could benefit from some renaming too to make it a bit more consistent with its USB counterpart.
There was a problem hiding this comment.
Going to leave this for a future update since pbsys starts a bit later. We should be fine but it means that the receive handlers get set up slightly later, so deserves a closer look.
This lets us start from a working REPL, and make everything work like the other hubs. Now that all communication goes through the proper sys/host module, we can also finally use the common mphalport for NXT.
This de-duplicates some of the system USB code by moving the platform agnostic parts to a common module. Continues the work started in #408 (it was merged, contrary to the GitHub indication).
Also introduces a stdout ringbuffer like we have in the Bluetooth driver so we can accumulate writes before sending. In MicroPython, every call to
printconsists of at least two calls to the underlying write code, so this helps.It is implemented for EV3 and the Spike hubs, as well as the Virtual Hub (though disabled in
pbdrvconfig.h). Empty hooks are also added for NXT, to be implemented in the future. The simulation driver gives a good overview of the hooks that are needed to port it to NXT.For EV3, this maintains the distinct descriptor for the response message, even though the common driver ensures that only one call to
pbdrv_usb_txis made at once. For the STM32 driver, it also still maintains the distinct buffer, but we probably don't need that anymore.