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 watchdog parameter-validation, and fix up misleading comments #1567

Merged
merged 1 commit into from
Dec 15, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions src/rp2_common/hardware_watchdog/include/hardware/watchdog.h
Original file line number Diff line number Diff line change
Expand Up @@ -30,17 +30,22 @@
extern "C" {
#endif

// PICO_CONFIG: PARAM_ASSERTIONS_ENABLED_WATCHDOG, Enable/disable assertions in the watchdog module, type=bool, default=0, group=hardware_watchdog
#ifndef PARAM_ASSERTIONS_ENABLED_WATCHDOG
#define PARAM_ASSERTIONS_ENABLED_WATCHDOG 0
#endif

/*! \brief Define actions to perform at watchdog timeout
* \ingroup hardware_watchdog
*
* \note If \ref watchdog_start_tick value does not give a 1MHz clock to the watchdog system, then the \p delay_ms
* parameter will not be in microseconds. See the datasheet for more details.
* parameter will not be in milliseconds. See the datasheet for more details.
*
* By default the SDK assumes a 12MHz XOSC and sets the \ref watchdog_start_tick appropriately.
*
* \param pc If Zero, a standard boot will be performed, if non-zero this is the program counter to jump to on reset.
* \param sp If \p pc is non-zero, this will be the stack pointer used.
* \param delay_ms Initial load value. Maximum value 0x7fffff, approximately 8.3s.
* \param delay_ms Initial load value. Maximum value 8388, approximately 8.3s.
Copy link
Contributor

@JamesH65 JamesH65 Dec 2, 2023

Choose a reason for hiding this comment

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

8.3 IIF the source clock is default?

Copy link
Contributor Author

@lurch lurch Dec 2, 2023

Choose a reason for hiding this comment

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

Well, you could argue that the \note If \ref watchdog_start_tick value does not give a 1MHz clock to the watchdog system, then the \p delay_ms parameter will not be in milliseconds. See the datasheet for more details. already covers that ? 🤷‍♂️

*/
void watchdog_reboot(uint32_t pc, uint32_t sp, uint32_t delay_ms);

Expand All @@ -63,7 +68,7 @@ void watchdog_update(void);
* \ingroup hardware_watchdog
*
* \note If \ref watchdog_start_tick value does not give a 1MHz clock to the watchdog system, then the \p delay_ms
* parameter will not be in microseconds. See the datasheet for more details.
* parameter will not be in milliseconds. See the datasheet for more details.
*
* By default the SDK assumes a 12MHz XOSC and sets the \ref watchdog_start_tick appropriately.
*
Expand All @@ -72,7 +77,7 @@ void watchdog_update(void);
* onto the RPI-RP2), then this value will be cleared, and so \ref watchdog_enable_caused_reboot will
* return false.
*
* \param delay_ms Number of milliseconds before watchdog will reboot without watchdog_update being called. Maximum of 0x7fffff, which is approximately 8.3 seconds
* \param delay_ms Number of milliseconds before watchdog will reboot without watchdog_update being called. Maximum of 8388, which is approximately 8.3 seconds
Copy link
Contributor

Choose a reason for hiding this comment

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

8.3 IIF the source clock is default?

* \param pause_on_debug If the watchdog should be paused when the debugger is stepping through code
*/
void watchdog_enable(uint32_t delay_ms, bool pause_on_debug);
Expand Down
4 changes: 3 additions & 1 deletion src/rp2_common/hardware_watchdog/watchdog.c
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@

/// \tag::watchdog_start_tick[]
void watchdog_start_tick(uint cycles) {
valid_params_if(WATCHDOG, cycles <= 0x1ffu);
// Important: This function also provides a tick reference to the timer
watchdog_hw->tick = cycles | WATCHDOG_TICK_ENABLE_BITS;
}
Expand All @@ -34,6 +35,7 @@ uint32_t watchdog_get_count(void) {
// tag::watchdog_enable[]
// Helper function used by both watchdog_enable and watchdog_reboot
void _watchdog_enable(uint32_t delay_ms, bool pause_on_debug) {
valid_params_if(WATCHDOG, delay_ms <= 8388); // i.e. floor(0xffffff / 2000)
Copy link
Contributor

Choose a reason for hiding this comment

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

Magic number, although it is commented. Move to a define? Only a nit though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could do, but I'm not sure if it's worth it when it only appears once in a debug-level assert?

hw_clear_bits(&watchdog_hw->ctrl, WATCHDOG_CTRL_ENABLE_BITS);

// Reset everything apart from ROSC and XOSC
Expand Down Expand Up @@ -103,4 +105,4 @@ bool watchdog_caused_reboot(void) {

bool watchdog_enable_caused_reboot(void) {
return watchdog_hw->reason && watchdog_hw->scratch[4] == WATCHDOG_NON_REBOOT_MAGIC;
}
}
Loading