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

Independent watchdog skeleton definitions #808

Merged
merged 12 commits into from
Aug 13, 2018

Conversation

sharmavishnu
Copy link
Contributor

@sharmavishnu sharmavishnu commented Jul 26, 2018

Description

  • These two files provide the method signatures and default weak implementations. Each MCU platform owner must use the provided method signatures and provide a strong implementation for the given MCU
  • Add option to enable (and manage) MCU Watchdog

Motivation and Context

Enable standard API for including support for independent watchdog for any given MCU and OS platform

How Has This Been Tested?

Used STM32F401RE nulceo board to test using the community target port. Concrete implementation of independent watchdog for the MCU STM32F401RE is also submitted in a separate PR against that repository. This will also act as a sample implementation

Screenshots

Types of changes

  • Improvement (non-breaking change that improves a feature, code or algorithm)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Signed-off-by: @sharmavishnu vishnusmoke@gmail.com

Weak function implementations for independent watchdog
Independent watchdog header file with required function declarations
@nfbot
Copy link
Member

nfbot commented Jul 26, 2018

Hi @sharmavishnu,

I'm nanoFramework bot.
Thank you for your contribution!

A human will be reviewing it shortly. 😉

@josesimoes
Copy link
Member

@sharmavishnu great start on this! 👏

Considering that:

  1. This is a feature to be used at the core level (meaning not exposed to managed developers)
  2. Keeping code size to the absolute minimum is high on the priority list
  3. This ideally should be setup almost automatically from CMake options and/or compiler defines

We can probably make some simplifications on what you have here.

On a first pass, these would be:

  1. Drop the functions Watchdog_SetMaxTimeout() and Watchdog_SetMaxTimeout(value)
    (the timeout should be a define)

  2. Drop Watchdog_Start(), Watchdog_IsRunning() and Watchdog_Stop() this feature is an either YES or NO, being able to change it during run time is asking for trouble and makes the management more complicated.

Plus some of the code you have in the community PR should be added here, because it's common to the STM32 targets and it's much efficiente to have it in a single place.

Let me know your thoughts on the above! 😃

Removed some APIs to simplify implementation.
Updated watchdog API default implementations after changes to the header file
Added default watchdog implementation file
Independent watchdog implementation for STM32 series MCU
Independent watchdog implementation for STM32 series MCU
Watchdog implementation file added to CMake list
@sharmavishnu
Copy link
Contributor Author

sharmavishnu commented Aug 6, 2018

Included all comments. The key changes are:

  1. Simplified watchdog API by having only "Init" and "Reset" functions
  2. Created generic watchdog implementation for STM32
  3. Anyone wanting to enable watchdog, must do the following:
  • Set HAL_USE_WDG as TRUE in halconf.h
  • Set STM32_WDG_USE_IWDG as TRUE in mcuconf.h
  • Define a new macro IWATCHDOG_CLK_TYPICAL and provide the typical clock value (in Hertz) in mcuconf.h (see datasheet of the specific MCU)
  • Define a new macro IWATCHDOG_TIMEOUT_MILLIS and provide the required watchdog timeout (in millis) in mcuconf.h
  • You are now ready. Call Watchdog_Init() method to initialize and start the watchdog and call Watchdog_Reset() regularly to ensure MCU does not reset.

(Note: Closed the PR on community target. No longer required to maintain watchdog implementation in specific implementations)

Copy link
Member

@josesimoes josesimoes left a comment

Choose a reason for hiding this comment

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

This looks very nice and clean.
Just take a look at the bellow comment and we should be able to merge this. 😉

*/
void Watchdog_Reset()
{
syssts_t sts = chSysGetStatusAndLockX();
Copy link
Member

Choose a reason for hiding this comment

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

Was checking the ChibiOS test code and there is no call to chSysGetStatusAndLockX as you are using here...
Do we need these?

josesimoes and others added 3 commits August 10, 2018 14:42
Forgot about the "break" statement in "for...loop". Corrected it.
Based on feedback, removed code for critical section in Watchdog_Reset. Since only the execution engine will be calling this and not user code, critical section within the reset is not required.
@josesimoes josesimoes changed the title Independent watchdog skeletal definitions Independent watchdog skeleton definitions Aug 13, 2018
@josesimoes josesimoes added Series: STM32xx Everything related specifically with STM32 targets and removed Status: under review labels Aug 13, 2018
@josesimoes josesimoes changed the base branch from develop to develop-watchdog August 13, 2018 08:30
@josesimoes josesimoes merged commit 828c6d9 into nanoframework:develop-watchdog Aug 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Common libs Everything related with common libraries Series: STM32xx Everything related specifically with STM32 targets Type: enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants