-
Notifications
You must be signed in to change notification settings - Fork 33
Feature EV3 firmware flash - minor improvements #2347
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
59e70c7
to
9d52d9d
Compare
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.
Thanks for continuing the work here. In addition to the comments I made, we need to fix the failing tests and write a descriptive commit message.
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.
Almost there. 😄
It would also be nice to clean up the commits into logical commits for the various fixes and features with appropriate messages. I don't mind doing that when I merge it if that is more convenient.
src/usb/sagas.ts
Outdated
if (!usbDevice.configuration && usbDevice.configurations.length > 0) { | ||
const [, selectErr] = yield* call(() => | ||
maybe( | ||
usbDevice.selectConfiguration( | ||
usbDevice.configurations[0].configurationValue, | ||
), | ||
), | ||
); |
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.
if (!usbDevice.configuration && usbDevice.configurations.length > 0) { | |
const [, selectErr] = yield* call(() => | |
maybe( | |
usbDevice.selectConfiguration( | |
usbDevice.configurations[0].configurationValue, | |
), | |
), | |
); | |
if (usbDevice.configuration === null) { | |
const [, selectErr] = yield* call(() => | |
maybe(usbDevice.selectConfiguration(1)), | |
); |
Since we control the firmware, we know that we always want configuration 1, which may or may not be the configuration at index 0.
And prefer strict equality checks instead of truthy/falsy unless there is a good reason for it.
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.
ok, corrected.
Any comment or assert to document this "magic" number?
Concern: code-logic wise this could even break, only works as you control the firmware.
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.
There are probably a few other hard-coded USB things that we need to clean up like this. Agree that it would be sensible to make a constant for this. But we can save that for later.
That is a fair comment. :D Apologies for my back-and-forth's. |
I'll fix up the commits how I like them and merge this. |
Fix issues with null vs. undefined. Based on DefinitelyTyped/DefinitelyTyped#73568
On some OSes, like macOS, the configuration may not be selected, so we need to make sure it is always set.
Dispatch an error in case there is no firmware URL. Normally this should never happen, but currently it does because we don't have EV3 firmware yet. The error is more useful than just spinning forever.
The v2.1 spec allows this, but up to now, Pybricks code didn't handle it.
Start listening for reply before sending command to avoid race condition of reply being received and action dispatched before we call take().
Implement unique message ID for EV3 firmware messages.
Add missing bits to get firmware flashing on EV3 working. Currently requires custom firmware since we don't have a packaged one yet.
8edc14a
to
311a833
Compare
For some reason, the type checker doesn't catch this, but FailToFinishReasonType.Unknown requires an error argument, otherwise we get an unhandled exception.
Still lots to do, but nice to handle it in smaller chunks like this. Thanks! |
Minor implementation changes to make pybricks/support#2318 work