Skip to content

Enable IWDG module for Nucleo-64 F446RE board. #234

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

Closed
wants to merge 30 commits into from

Conversation

ghent360
Copy link
Contributor

@ghent360 ghent360 commented Apr 2, 2018

Not sure why it was not enabled in the first place. It would probably work fine for other boards
as well.

enabled in the first place. It would probably work fine for other boards
as well.
@fpistm
Copy link
Member

fpistm commented Apr 3, 2018

Hi, @ghent360
There is no specific reason.
At the beginning we targetted to be Arduino Uno "like", so only basic feature was enabled, I guess.
User can enable HAL module, modifying the hal conf, or more simply adding a build_opt.h file at sketch level with -DHAL_IWDG_MODULE_ENABLED
Anyway, I see no reason to enable it by default.

In general, it could be interesting to provide small Arduino API like to to use dedicated feature.

@ghent360
Copy link
Contributor Author

ghent360 commented Apr 3, 2018

Hi, @fpistm
Thank you for the consideration. Yes the build_opt.h file does work.
How does this look for an API start:

https://github.com/ghent360/Marlin/blob/bugfix-2.0.x/Marlin/src/HAL/HAL_STM32F4/watchdog_STM32F4.h

Or would you rather have Arduino style C++ API?

Watchdog.begin(uint16_t reload_time /* seconds */); // starts the watchdog
Watchdog.reload(); // call periodically to reload and prevent reset

@fpistm
Copy link
Member

fpistm commented Apr 3, 2018

welcome. It's an community project. Any contribution are welcome. ;)
About API, it's open. C or C++ are welcome. I think the goal is to have something generic and user friendly.
Probably, C++ is not required to avoid use an instance or maybe both could be proposed...
C++ one using the C API?
Do not hesitate to propose one, why not based on Marlin one.

@ghent360
Copy link
Contributor Author

ghent360 commented Apr 4, 2018

I reverted the HAL config change since the build_opt.h file can serve the same purpose. I added a Watchdog library and an example to expose the IWDG component.

@ghent360
Copy link
Contributor Author

ghent360 commented Apr 6, 2018

@fpistm any thoughts on the API proposal?

@fpistm
Copy link
Member

fpistm commented Apr 6, 2018

Hi @ghent360,
first look seems fine.
I will deeper review it in the coming weeks. Currently, I'm reviewing/testing RTC and Low Power which is a huge task ;)

@fpistm fpistm added enhancement New feature or request New feature labels Apr 6, 2018
@fpistm fpistm self-requested a review April 6, 2018 05:05
@fpistm fpistm force-pushed the master branch 2 times, most recently from 5f641c5 to cf54e13 Compare May 12, 2018 16:06
@fpistm fpistm added this to the 1.2.1/1.3.0 milestone May 28, 2018
@fpistm fpistm modified the milestones: 1.3.0, 1.3.1 Jun 13, 2018
@fpistm
Copy link
Member

fpistm commented Jun 13, 2018

I need to do more tests before release it so I will not have time to validate it properly for 1.3.0. This will be part of the 1.3.1

@fpistm
Copy link
Member

fpistm commented Jul 31, 2018

I've reviewed this PR and done tests across several STM32 Series.
Basically, it works for series which do not have Window feature on those having this not worked.
Library format is not fully respected: src/ folder, library version have to be semver x.y.z.
I will rename it IWatchdog to differentiate with WWDG (when implemented).
New API could be added to know if the system has resumed from IWDG reset.
And probably handle also reset < 1ms (handle prescaler value 4/8/16) even if LSI accuracy is not very good. (I remember it is possible to use a timer connected to LSI to measure LSI freq but this could/will be an other enhancement, primary goal of this PR is to provide IWDG access :))
Also use LSI_VALUE typical value to define the best combo prescaler/reload value (example for F1, LSI is 40KHz).
Maybe using C++ could be fine to handle default method parameters and also using LL.
I need to think about all of this to handle it in the best way.
Any suggestions are welcome.

Copy link
Member

@fpistm fpistm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#292 will replace this PR

@fpistm
Copy link
Member

fpistm commented Aug 7, 2018

#292 merged. This one can be closed.
Thanks @ghent360

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants