-
Notifications
You must be signed in to change notification settings - Fork 119
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
Make the default flashing frequency target specific #389
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I think FlashFrequency::default()
usage is better (as in esp8266 module) and more future-proof (if this code will ever be refactored to use ..Esp32Params::default()
), but it's up to decide :)
I've browsed the changes on my phone and I can't really understand your
suggestion, do you mind elaborating? I'm any casi, tomorrow morning I will
have a better look at it!
Thanks for the feedback!
…On Mon, Apr 17, 2023, 20:32 Max Wase ***@***.***> wrote:
***@***.**** commented on this pull request.
Personally, I think FlashFrequency::default() usage is better (as in
esp8266 module) and more future-proof (if this code will ever be refactored
to use ..Esp32Params::default()), but it's up to decide :)
—
Reply to this email directly, view it on GitHub
<#389 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADCTYYK4VCNOW6RCMHGZON3XBWED3ANCNFSM6AAAAAAXBBB32E>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Oh, sorry for unclarity 😄, I mean replacing of |
Ahh! I've tried this yesterday, but since
And I don't know/couldn't find any easy solution to this |
Oh, didn't spot that, hope we will get it someday! Other than that, LGTM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for fixing this!
I've implemented the third solution proposed by @maxwase in #375:
flash_freq
toEsp32Params
so we can define a default flashing frequency per target.