Skip to content
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

Add an optional callback trait which can be implemented and provided to most flashing functions #333

Merged
merged 1 commit into from
Jan 11, 2023

Conversation

jessebraham
Copy link
Member

Hopefully this will make espflash a bit more usable as a library.

@bugadani does this seem reasonable enough to you? Would love to hear any feedback you might have.

I suppose this is sort of an extension of #283 as well, so @svenstaro will this still work for your use case as well? Sorry for changing this again 😁

Closes #310.

@bugadani
Copy link
Contributor

Thanks!

This looks awesome, although I have two small comments:

  • I think you might want to pass in the impl ProgressCallbacks type by value. I think that if I really need a reference to some object, I might implement the trait for the reference myself. Explicitly borrowing sounds a bit unnecessary to me.
  • I think I would replace the optional arguments with an empty ProgressCallbacks implementation, which would allow cleaning up some of the progress report sites (like write_segment). Although, I don't know if this would be a good idea or a complete antipattern instead 🤷‍♂️

@jessebraham jessebraham marked this pull request as draft January 10, 2023 16:37
@svenstaro
Copy link
Contributor

Should be fine for my purposes. Certainly improves the API.

@jessebraham
Copy link
Member Author

@bugadani I briefly poked at this and tried to get it working with impl ProgressCallbacks but I kept running into issues regarding object safety. I am pretty busy with other projects right now so I can't really spend much time on this for a bit. If you're able to build on top of this that'd be great; otherwise, whenever I get back to it I will either update it myself or just merge it as-is. Even though the API isn't ideal it does at least work in its current form.

@bugadani
Copy link
Contributor

Oh, please don't feel like my comments are anything more than mere wishes. I'm pretty happy with the PR as is :)

@jessebraham
Copy link
Member Author

Okay, glad to hear that. In that case I'm just going to merge this I suppose, we still have the opportunity to tweak it if needed.

@jessebraham jessebraham marked this pull request as ready for review January 11, 2023 16:49
@jessebraham jessebraham merged commit 421a8dc into esp-rs:main Jan 11, 2023
@jessebraham jessebraham deleted the feature/progress-callbacks branch January 11, 2023 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

espflash lib: There is no way to display progress unless the cli feature is enabled
3 participants