search

dfu-rs / dfu-core

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

[FR] method to set the start address for download #7

Closed Qyriad closed 2 years ago

Qyriad commented 2 years ago

Currently, it doesn't seem that DfuSync offers a method to set the starting address (equivalent to dfu-util's -s/--dfuse-address argument). I see a command for setting the address in dfu_core::download, but there doesn't seem to be a public interface for doing so, unless I'm mistaken?

In either case, a set_address() method on DfuSync would be amazing, if possible 🙂

gnxlxnxx commented 2 years ago

to me It looks like you can set the address on creation via DfuSync::new. I could be wrong though.

cecton commented 2 years ago

hah! ... :thinking: did I forget something....

:thinking: ....

Reading the code it seems the address is not settable by the user but is based on the config descriptor.

You can see that here: https://docs.rs/dfu-libusb/0.2.0/src/dfu_libusb/lib.rs.html#145-148

It's been a long time so I'm not sure my memory is correct: I think I have tested writing manually on a segment of memory I want and it failed. The data was not properly written on the device for some reason. So I settled that by only allowing to write on the address in the config descriptor. But I'm not certain.

However it is possible that this was an issue for my particular device. So I guess we could add a method set_address on DfuSync to override the address afterwards.

I really depend on your experience of you both here because I'm not originally an embedded developer. You need to make sure that:

  1. changing the address manually is a relevant use case for the user: I assume it is otherwise this ticket wouldn't be open... but yeah, challenge it just in case
  2. it won't lead to undefined behavior: I believe in what I tested I got weird results, if that's the case it's still okay to add a method to override the address but it should come with a warning in the documentation (if it really is doing weird things)
cecton commented 2 years ago

to me It looks like you can set the address on creation via DfuSync::new. I could be wrong though.

yes but the user don't usually create that manually. It's created through an implementation (DfuLibusb in this case)

cecton commented 2 years ago

@Qyriad if you're happy with the solution in #9 I can merge that and make a release if you need.

Qyriad commented 2 years ago

As for whether behavior like this is technically allowed by the spec, I truly have no idea, but as for whether its useful, I can think of a few example cases:

  1. The device isn't properly DFU-compliant and doesn't have the right address in its string descriptor (and when it comes to USB, incorrect behavior is extremely common)
  2. The firmware binary is linked to the wrong offsets
  3. You want to write only part of the firmware

I think some mix of 1 and 2 is my use case at the moment. I'll check #9 on my hardware later today, but a glance on the diff looks pretty good, thank you!

Qyriad commented 2 years ago

Whoops! Sorry, I got a bit distracted. But I did test #9 on my hardware, and I can confirm it does indeed work, so it should be mergeable. Thank you!

cecton commented 2 years ago

I completely forgot xD

Ok fixed by #9 then

cecton commented 2 years ago

Published 0.3.0 (there is a small breaking change in the API for the progressbar but shouldn't be a problem for anyone I think)

Qyriad commented 2 years ago

Thank you!