Add DFU Class per Version 1.1 Spec #754
Conversation
|
great work as usual, please give me a bit of time, I will try to review it asap ( in the middle of something else 😅 ) |
There was a problem hiding this comment.
Thank you very much for your PR. Sorry for late review, I haven't fully go through it yet. Though there is one thing I spotted. Since DFU mode & runtime is mutual exclusive. I would like to have it as separated driver. So that application that enable the rutnime does not need to also include the DFU mode which require more RAM & ROM. Could you separate the dfu functionalities into dfu _device.h/c, we will leave the dfu_rt code as it is (common enum/typedef move to dfu.h is spot-on)
PS: I will do more in-depth review soon enough, though separated it first will definitely help.
I will separate out class functionality of DFU mode from RT. |
Fixes bug where the app callback was getting the length of the status request transfer rather than the length of the data stage payload. TODO: Right now this returns the expected length, when it really should be returning the transfer length.
Thanks, I will do more review this week, need to pull out the DFU specs, kind of forgot it entirely |
With the runtime and mode portions in separate classes, a single application should only be building with one or the other enabled. In some applications both might be desired at build time. The CFG_TUD_DFU_RUNTIME_AND_MODE option creates a DFU class, which asks the application which mode to initialize to. This allows a runtime change between RT and DFU mode, by just reinitializing tusb.
There was a problem hiding this comment.
Thank you very much for your PR. I haven't go through all the changes just yet. Though I notice several changes that we could address first with DFU Runtime, still haven't looked at actual DFU mode yet.
- There is couple typo when comparing runtime state with
DFU_REQUEST_DETACH, should beAPP_DETACH - Bitwise result must not be equal-check with 1, it is good as just bitwise alone
- the updated dfu runtime is more complicated than it should be. I think the existing/previous code is good enough.
PS: Adding a new driver is lots of works, we may need to go through a few rounds of reviews before merging. Please be patient.
Returns the RT driver to the function state of previous iteration, which did not support the will_detach. Behavior should be fine without this feature. This removes much of the added bloat to track state, and handle requests in the APP_DETACH state which is no longer required. Removes the optional bloat added to the RT driver, such as responding to GETSTATE requests. Fixes the DFU Mode to extract the attr bits from the functional descriptor when opened. Fixes some incorrect bitwise if checks. Also, updates some naming of functions to be consistent with the rest of the library.
|
I am expecting that you would like the nonstandard requests removed from the DFU mode, but I will wait until your review before I make this change. To adhere to the spec, the finite state machine is verbose, with a fair bit of duplication. I opted for readability rather than compactness, which is why some things, such as get_state and get_status requests are in most states, rather than being checked at the request level. |
|
Thank you very much for you update, I will review it asap :) |
There was a problem hiding this comment.
Thank you for your PR again. There are only several comments, but all is minor (except the timeout change), not really require much changing. Besides that I have 2 major requests
- Could you simplify the
dfu_modeto justdfu(both in API and file name). I think it will be simpler and easier to read. If you think it is too much of a hassle, then I could help with the rename - As part of adding new class, it typically requires to have an example to (a) to test the driver and (b) demonstrate to user on how to use the API. Example is also my excuse for not writing fancy doc, since I simply just point user to example 😄 . In this case, we don't have to do any actual flashing. We only need to dump/print out the download data, and response to upload as well as other requests from the dfu-util.
PS: I am sorry if the review is a bit overwhelming, I try my best to do so. For some of the changes, If anything is unclear, or you have any other idea feel free to suggest. I am open to all dicussion.
src/class/dfu/dfu_mode_device.h
Outdated
| // Application Callback API (weak is optional) | ||
| //--------------------------------------------------------------------+ | ||
| // Invoked when a reset is received to check if firmware is valid | ||
| bool tud_dfu_mode_firmware_valid_check_cb(void); |
I can do this. It's funny that you asked for that. When I separated the RT and DFU mode portions I had everything named DFU, and then added MODE so it was consistent with RT. I will change things from dfu_mode to dfu as I go through the rest of your comments.
Yes, adding an example was on the TODO list. My testing with dfu-util was with dummy handlers that just printed the contents being downloaded. The upload test was to a file that I could just open which would have the arbitrary string "Hello world" I will add this demo once I have addressed the other comments. |
Actually, I meant to split the runtime from the actual DFU mode, have them as separated drivers. Though words can be misleading, English is not my 1st language after all. That is ok if you don't want to make the rename, I could do it myself. Not a deal breaker at all.
Thanks, that would be helpful |
Moves dfu_req_dnload_reply to ACK stage of a DNREQUEST. Removes unneeded variables due to other simplifications.
|
Thank you @xmos-jmccarthy for addressing previous feedback, Let me know if you want me to review the updated code, or wait until you push the dfu example. |
|
Please review the changes in the latest commit. There are a few unresolved comments that I would like more feedback/consideration on before I make them. Once the class driver is complete I will add in an in repo example. |
| } | ||
| } | ||
|
|
||
| static bool dfu_state_machine(uint8_t rhport, tusb_control_request_t const * request) |
There was a problem hiding this comment.
IMHO, dfu_state_machine() is written more complicated than it should be. To be honest, I don't know what would suggest here. Maybe when adding example we could understand this better.
There was a problem hiding this comment.
Yes, so this concern one of my prior comments. I am very open to changing it, but here is some background for how it came to be.
I opted for the current setup where there is a very verbose state machine, but it is explicit in the behaviour to expect on any given request. An earlier iteration had all behaviour inside of dfu_moded_control_xfer_cb. This was incredibly difficult to follow, since each request ended up containing a switch for every single possible state, where most resulted in a stall.
I then migrated to a setup where some common commands such as DFU_REQUEST_GETSTATUS and DFU_REQUEST_GETSTATE where handled in dfu_moded_control_xfer_cb in the bRequest switch, and the rest where in the dfu_state_machine function. This was better, but there still was some ugliness due to DFU_MANIFEST, DFU_MANIFEST_WAIT_RESET, and DFU_DNBUSY supposed to stall when these requests occur or DFU_MANIFEST_SYNC having different behaviour on a DFU_REQUEST_GETSTATUS if DFU_FUNC_ATTR_MANIFESTATION_TOLERANT_BITMASK is set.
There was a problem hiding this comment.
I see, it is your point of view for implementation. Though it is still a bit complicated for my taste. However, you don't have to change it now. We will see how it turn out when adding the example. And I will try to move code around if needed as follow up later on. I do have some renaming in my mind. It is too minor mentioned in reviews.
| _dfu_state_ctx.state = DFU_MANIFEST; | ||
| dfu_req_getstatus_reply(rhport, request); | ||
| } else { | ||
| if ( tud_dfu_firmware_valid_check_cb() ) |
There was a problem hiding this comment.
I think we should just remove this tud_dfu_firmware_valid_check_cb()
Side note: we don't really need to check for the MANIFESTATION_TOLERANT here. The reason is that if we could have CPU checking this code, we already able to communicate via USB and therefore imply the MANIFESTATION_TOLERANT = 1 already.
There was a problem hiding this comment.
This firmware check determines whether a DFU_GETSTATUS results in dfuMANIFEST, signifying that the firmware is invalid/inprogress, or dfuIDLE if manifesting was completed, from table A.2.7 in the V1.1 spec.
WIth the other changes made, primarily the reset behaviour, the manifestation portions don't really offer anything anymore.
If the class is only responsible for the setup and transfer of firmware, and we want to leave it up to the application for the rest, then the manifestation can be removed entirely. In that case, I would propose that after the last DNLOAD request, with length 0, the class enters the dfuMANIFEST-SYNC state. The final host side DFU_GETSTATUS request would change the class state to dfuIDLE, where the class would then sit forever, leaving it up to the app to independently finalize it's work and reboot.
Removal of DFU_MANIFEST and DFU_MANIFEST_WAIT_RESET would also allow some other areas to be simplified. DFU_GETSTATE could be checked in the request switch without it looking too messy since dfuDNBUSY would be the only remaining internal state to stall on.
There was a problem hiding this comment.
Yeah, that is the approach. Bootloader application is the one in charge of performing checksum, jump app. From my experience working with a couple of bootloader project tinyuf2 and Adafruit_nRF52_Bootloader . DFU class will only be one of supported transport together with msc (drag and drop firmware), HID, WebUSB, CDC, or even BLE/WIFI OTA. Typically each transport will let the bootloader know when all the data is transfer, bootloader witll finish all the writing, then do some checksum, reset peripheral, and reboot too application its-self. It won't even need any further help from host llike GETSTATUS (from most MCU I have worked with).
| } | ||
| } | ||
|
|
||
| static bool dfu_state_machine(uint8_t rhport, tusb_control_request_t const * request) |
There was a problem hiding this comment.
I see, it is your point of view for implementation. Though it is still a bit complicated for my taste. However, you don't have to change it now. We will see how it turn out when adding the example. And I will try to move code around if needed as follow up later on. I do have some renaming in my mind. It is too minor mentioned in reviews.
Update API description.
|
I added an example app for review. I added a separate example, rather than updating the RT example. The reason for this was to avoid requiring that the app reboot/support dynamic configuration (my device does not). dfu-util will give a warning that the device already is in dfu mode, but will still perform the upload or download requested. I unfortunately do not have my platform ported to run within the TinyUSB repo so I was unable to test, as we discussed earlier. I have tested upload and download of real firmware on my platform to verify the transport correctness. As discussed earlier, DFU_MANIFEST isn't needed, so I am still in favor with removal to simplify the state machine. I can do this is you'd like, or you can during your move around/standardization. |
|
Thanks for the update, Adding it as separated example is spot on. DFU runtime is application, while DFU is actually bootloader. I am more than welcome anything that could help with the simplification (the stack is too complicated already). I will do an actual test and review as soon as I could |
|
sorry I have been super busy with lots of work for Arduino TinyUSB Lib for rp2040. I will try to check this out asap (soon).. |
There was a problem hiding this comment.
superb! thank you @xmos-jmccarthy very much for the PR. I am really appreciated your effort and patient throughout the whole PR reviews. It has been a bit busy due to the release of rp2040 in recent time. And I couldn't keep up with all the fix and pr. The example work great with both download and upload. Bravo !!! 🎉 🎉 . And it is good for the merge now. Later on when integrating into another bootloader, I will do more actual testing on hardware. Should there is any improvement, I will tag you for review/feedback therefore please kindly accept the collaborator invitation if you accept.
PS: I have updated the PR to fix build issue on CI.
Please review. I have some final testing to complete, and then some additional cleanup I would like to do. I attempted to best follow the existing structure and styling. Please let me know if anything is misplaced, desired debug missing, or any other general hygiene issue.
Describe the PR
This PR completes the DFU Class, resolving feature Issue #29. It extends the prior DFU Class stub, which implemented only a DFU RT detach flow.
Additional context
The class has been validated with dfu-util to complete both download and uploads. There are numerous callbacks that the application must implement, due to the application specific nature of DFU, so I am unsure on what to change the default demo to. After my testing and cleanup per your requests are complete I will determine how to change the existing demo.
Merging of this PR would invalidate PR #519.
Future Enhancements
One area of improvement is in how the upload and download data step is handled. Currently, the DFU driver passes the pointer to it's internal state buffer of CFG_TUD_DFU_TRANSFER_BUFFER_SIZE bytes. This is functional, but if I have time I would like to add an alternative zero copy mode so the application can provide this pointer. That way there is 1 less memcpy, as usbd_control_xfer_cb would be doing it's memcpy directly into the application buffer. The current setup is safer, as it guarantees the buffer is large enough (assuming the usb descriptor has a transfer size <= CFG_TUD_DFU_TRANSFER_BUFFER_SIZE).