-
-
Notifications
You must be signed in to change notification settings - Fork 80
[EV3] Implement Pybricks USB protocol #361
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
With this in place (and possibly some fixes in pybricksdev? I was noticing some strange issues) we might be able to call this an early alpha release? |
Can you elaborate? (Or send a PR) |
dlech
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not the most thorough review and I won't be able to test until sometime next week, but looks pretty good to me.
lib/pbio/drv/usb/usb_ev3.c
Outdated
| static uint32_t prev_status_flags = ~0; | ||
| static uint32_t new_status_flags = 0; | ||
|
|
||
| (void)ep1_tx_stdout_buf; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This silences a build failure due to warnings-treated-as-errors in the intermediate commit. This is because the variable is never used until stdout is implemented.
|
|
||
| // Handle timeouts | ||
| is_transmitting = usb_tx_response_is_not_ready || usb_tx_status_is_not_ready || usb_tx_stdout_is_not_ready; | ||
| if (was_transmitting && is_transmitting) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps can use pbio_oneshot()?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason I am not is because the code needs to handle both the rising edge case and the "continuously active" case.
| // Handle timeouts | ||
| is_transmitting = usb_tx_response_is_not_ready || usb_tx_status_is_not_ready || usb_tx_stdout_is_not_ready; | ||
| if (was_transmitting && is_transmitting) { | ||
| if (pbio_os_timer_is_expired(&tx_timeout_timer)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we checking for timeout before setting the timer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the timer gets set in the else if later, which this code has to pass through before both flags can become true.
|
Amazing!
At a glance, Pybricksdev seems to still always start slot 0 rather than start the selected slot. So code loaded to other slots does not appear to run. It does run if you start them with the button.
Let's wait a while. Anyone who is able to run it in the current state is probably also able to grab the nightly release. I'd like to turn off some of the developer hacks (the hard reset; sensor 1 disabled, repl on 0) and get the official devices enabled (except I2C). With the rapid progress here, I think I'll put aside some of the internal BLE refactoring and return focus to EV3 sensors and UX. I was also occasionally getting this: |
|
Forgot to reply to this, but
seems to describe the bulk of the strangeness I was experiencing. The other issues included:
I have cleaned up most of the review comments though. |
This is happening because pybricksdev is not getting the status update until after starting the program. See pybricks/support#2306 So we could fix this in Pybricksdev, but it also sounds like the EV3 firmware is doing something slightly different than SPIKE? |
|
Yes, I was hoping to merge these first and then make fixes as needed. I was waiting for @dlech to get back, unless you want to go ahead and click the button. |
These transfers allow reading "characteristics" which would otherwise be exchanged over BLE GATT. They contain information about the firmware version and hub capabilities. Also move the bRequest values into pbio/protocol.h so they can be shared
In order to be able to detect disconnection, we cannot force device mode in the PHY. Forcing device mode overrides the analog comparators used to detect VBUS. Not forcing device mode should be fine because device mode is the default (enabling host mode requires setting DEVCTL.SESSION explicitly, and the ID pin is explicitly disconnected on the EV3 PCB.)
This handles Pybricks commands, status flag reporting, and timeouts. stdout is not yet handled.
This replaces the old stopgap stdout logic with stdio over USB
Without this timer, if the hub state doesn't change and the user program doesn't output anything, the hub won't be able to detect that the host software has closed (because it never sends anything which could time out). Keeping this state reasonably up-to-date is important for the hub auto-power-off functionality.
|
Done, thanks! Yes, let's build on this in follow up commits as needed. |
This fully implements the rest of the Pybricks USB protocol, allowing
pybricksdevto run.