OTA #531
OTA #531
Conversation
| /// | ||
| /// Returns `true` if the information was successfully fetched. | ||
| /// Returns `false` if the firmware data has not been loaded completely yet. | ||
| pub fn fetch(&self, data: &[u8], info: &mut FirmwareInfo) -> Result<bool, EspIOError> { |
There was a problem hiding this comment.
@ivmarkov I'm currently trying to port the EspFirmwareInfoLoader to EspFirmwareInfoLoad.
While i understand the performance changes, i have a few question about this changes.
Why is FirmwareInfo passed as mutable reference, in my point of view it would be much easier to just return Return<Option, EspIOError> instead of the bool.
Also is there any reason why fetch takes &self?
There was a problem hiding this comment.
@ivmarkov I'm currently trying to port the EspFirmwareInfoLoader to EspFirmwareInfoLoad. While i understand the performance changes, i have a few question about this changes.
Why is FirmwareInfo passed as mutable reference, in my point of view it would be much easier to just return Return<Option, EspIOError> instead of the bool.
That's how it used to be in the previous loader.
However, FirmwareInfo has a relatively large footprint of ~ 500 bytes. Why I designed it like this is another topic, but fixing embedded-svc (from where it comes) is not a topic right now. Therefore, it is not ideal if it is passed with moves as it occupies a large stack size then, especially in an async context, where futures' memory layout is not optimized w.r.t. input arguments (they are duplicated).
Also is there any reason why fetch takes &self?
No particular reason, but doesn't hurt either, as FirmwareInfoLoad is currently an empty struct, so the compiler would optimize away the &self anyway.
There was a problem hiding this comment.
That's how it used to be in the previous loader.
The performance optimatisations i noticed where that EspFirmwareInfoLoader did have a copy of the buffer, while EspFirmwareInfoLoad just works an a slice.
Good point with the copy on return, however for non async cases, the compiler should complety optimize that and the api is much nicer. It dosn't help that Firmwareinfo does not have a default implementation, but that's a embedded_svc topic.
No particular reason, but doesn't hurt either, as FirmwareInfoLoad is currently an empty struct, so the compiler would optimize away the &self anyway.
Sure it doesn't hurt, but it's confusing for user of the API, at least thats my experience.
There was a problem hiding this comment.
however for non async cases, the compiler should complety optimize that and the api is much nicer
I wish I was also as confident as you are in that. Look here.
No particular reason, but doesn't hurt either, as FirmwareInfoLoad is currently an empty struct, so the compiler would optimize away the &self anyway.
Sure it doesn't hurt, but it's confusing for user of the API, at least thats my experience.
If nothing else, these changes were already released, and I don't think the API breakage of removing &self is worth it.
There was a problem hiding this comment.
Thanks for the insights.
I agree that a new release is not worth changing the &self.
Implements a new type
EspFirmwareInfoLoadthat has a reduced memory consumption, as per the docs.