dfu-rs / dfu-core

Sans IO core library (traits and tools) for DFU
16 stars 5 forks source link

Doesn't work with devices not implementing STMs dfu extensions #17

Closed sjoerdsimons closed 1 year ago

sjoerdsimons commented 1 year ago

I'm looking through existing crates implementing the DFU protocol, in particular to use with a TI SoC and u-boot; This one thusfar seems one of the nicest, unfortunately it uses the STM DFU extensions which make it unusable with other devices..

For example the special meaning of wBlockNum 0 (and 1) of DFU_DNLOAD is STM specific and doesn't work for anything else; A "standard" device will just interpret it as an upload of block 0 :)

Is supporting devices purely implementing DFU 1.1 of interest for this crate?

cecton commented 1 year ago

Of course! My idea was to make something very generic that can be integrated anywhere.

Unfortunately I don't do much embedded development nowadays so I haven't work on this for a very long time. I would gladly review any pull request though!

sjoerdsimons commented 1 year ago

great i'll see if i can put something together for that

cecton commented 1 year ago

Let me know when you think we are ready for a release

sjoerdsimons commented 1 year ago

Let me know when you think we are ready for a release

now ? :) At least i have nothing outstanding - I'll just note that i've only practically tested it against a DFU 1.1 device as i don't have dfuse capable devices (the mock test works ofcourse though :p)

cecton commented 1 year ago

To be sure I tested on my device and it's not working. I will investigate later but it delays the release a bit...

cecton commented 1 year ago

Nervermind it started all of a sudden

cecton commented 1 year ago

I think there is an issue because after your change, the download starts ~45sec after connection.

Without your change, the download starts ~12 sec after connection.

So I will still need to check the logs and investigate to see if there is really an issue or if it's normal somehow.

sjoerdsimons commented 1 year ago

No rush :) Happy to take a look at what might be going on as well tonight, if you'd like me to please add log traces of both the old and new code

cecton commented 1 year ago

8422 lines vs 8733. It's doing more work after.

8 positions erased before vs 26 after

As I suspected, you probably improved the thing

before.log after.log

cecton commented 1 year ago

Last position erased after: 135036928 Last position erased before: 135004160

Start position erased: 134217728

The data I am writing on the device is exactly 841644 bytes

So before we erase: 786432 vs after: 819200

Looks like they are both under the file size.... :thinking: ah but maybe there is a block size so it's the position of the starting of the block that is erased.

Block size seems to be 32768

So before we erase 819200 vs after: 851968

So it looks like it's better with your change! It might be worth writing a test about this...

cecton commented 1 year ago

On the other hand... there is only one extra block erased. That doesn't justify the 8 vs 26 difference of log lines for erasure. So there is another issue....

cecton commented 1 year ago

Positions erased before are:

Lets diff this:

--- /tmp/a      2023-01-25 17:52:23.761775318 +0100
+++ /tmp/b      2023-01-25 17:52:20.328171104 +0100
@@ -3,6 +3,24 @@
 134283264
 134316032
 134348800
+134381568
+134414336
+134447104
 134479872
+134512640
+134545408
+134578176
+134610944
+134643712
+134676480
+134709248
 134742016
+134774784
+134807552
+134840320
+134873088
+134905856
+134938624
+134971392
 135004160
+135036928

Oh yeah the size of the block actually vary on my device!

[32768, 32768, 32768, 131072, 262144, 262144, 262144, 262144, 262144, 262144, 262144]

So probably the erased command is ran for an entire block? If this is the case, it means that actually the code after is wrong because it's assuming the blocks are all 32768 long.

cecton commented 1 year ago

@sjoerdsimons what does the specs says about the erase command? Does it erase one of those block entirely depending on the size of each block or is there a constant value or something?

sjoerdsimons commented 1 year ago

right that's due to the last refactoring i did to pull out the page size once early on ; will look at adding a test for this

sjoerdsimons commented 1 year ago

@cecton for completeness what's the interface description of your device ooi

sjoerdsimons commented 1 year ago

So probably the erased command is ran for an entire block? If this is the case, it means that actually the code after is wrong because it's assuming the blocks are all 32768 long.

should be fixed with #21 :)

cecton commented 1 year ago

FunctionalDescriptor { can_download: true, can_upload: true, manifestation_tolerant: false, will_detach: true, detach_timeout: 255, transfer_size: 2048, dfu_version: (1, 26) }

Found DFU: [0483:df11] ver=2200, devnum=12, cfg=1, intf=0, path="3-1", alt=0, name="@Internal Flash /0x08000000/04032Kg,01128Kg,07*256Kg", serial="327032723438"

cecton commented 1 year ago

I confirm the issue is fixed. Thanks a lot!