drivers: watchdog: Watchdog API redesign [RFC] #1260

m-kru · 2017-08-25T14:48:27Z

New API enables setting watchdog timeout in the unit of microseconds.
It is possible to configure watchdog timer behavior in CPU sleep state
as well as when it is halted by the debugger.
It supports watchdogs with multiple reload channels.

Fixes #3994

Signed-off-by: Michał Kruszewski [email protected]

m-kru · 2017-08-25T14:50:32Z

include/watchdog.h

+ * timeouts - pointer to an array storing timeouts values in microseconds
+ *	      for all enabled channels. If watchdog supports multiple channels
+ *	      but only one counter reload value then the first value from the
+ *	      array will be taken.


I still wonder if such high resolution for watchdog is required. Maybe it would be sufficient to use u16_t and milliseconds.

Kernel timer API is using milliseconds so indeed we would be more consistent sticking with this unit. Also watchdogs often work with slow / imprecise clock, microsecond resolution is not required.

My opinion is just the opposite, let use [1 us] resolution. Usually WDT is clocker from several dozen or several hundred KHZ - so resolution is always muh more than [1ms]. Regards precisin: On nRF5x it is drive form 32768 Hz and it can be the crystal oscillator, so it has precise timing. I believe some other SoC has similar capabilities - why to lock these capabilities. I see sense in keeping 1us resolution.

Watchdog clocks are designed to be reliable not precise. Typically these are a very imprecise RC clock sources, often as a fallback when a crystal fails. But even if we had a 32768 crystal oscillator only microsecond precision is not required. We won't use watchdog to measure time. Resolution of watchdog is strictly connected with the system tick. It will never be smaller than a tick length. Watchdog timeout value lays typically between ~10ms to some seconds. It should be as small as possible but has to include all the uncertainties: inaccuracy of the source clock, scheduler. There is always safety margin added to the timeout value and it will never lay in microseconds range. If Zephyr allowed to set system tick to value smaller than 1 ms then the watchdog should do it too but it the same way kernel API does. Let's keep APIs consistent.

nvlsianpu · 2017-08-28T07:59:31Z

include/watchdog.h

-	u32_t timeout;
-	enum wdt_mode mode;
-	void (*interrupt_fn)(struct device *dev);
+	u32_t *timeouts;


For me 1 us is the good resolution. Of course the realized timeout value for timeout requested is depend on the hardware. Is there any way to get the real timeout? Is it expected to be retrieved by get_config call?

timeouts with an 's'? [edit] Missed the comment detailing this attribute...

@nvlsianpu No, there is no way to get the real timeout. Driver implementation should approximate to the closest possible value or maybe to the closest higher value? get_config will be probably removed as no one can find any use case for it right now.
Maybe *timeouts table should be in/out parameter, after leaving the function it would store the real set values?

@m-kru Works for me as well.

mnkp

Thanks @m-kru for working on the new watchdog API. It certainly needs a refreshment.

In addition to my comments below I have one extra remark. As captured by this API a watchdog timer needs to be reloaded before a fixed amount of time has elapsed. However another common feature of a watchdog module is to provide a minimum time before which the watchdog timer is not allowed to be reloaded. I.e. the watchdog timer can only be reloaded in a time window with predefined minimum and maximum values. This is not supported by this API.

Could you please give an example of a SoC with multi-channel watchdog?

mnkp · 2017-08-28T12:14:26Z

drivers/watchdog/Kconfig

@@ -1,23 +1,38 @@
 # Kconfig - Watchdog configuration options
 #
-#
-# Copyright (c) 2015 Intel Corporation


The file was modified, we probably should not remove the old copyright, feel free to add new.

mnkp · 2017-08-28T12:17:07Z

drivers/watchdog/Kconfig

 menuconfig WATCHDOG
 	bool
 	prompt "Watchdog Support"
-	default n


What was the reason to remove this line?

@mnkp Lines with default n are not required. n is the default where there is no default line. There is not need to make file size greater than necessary.

mnkp · 2017-08-28T12:48:06Z

include/watchdog.h

+ * @brief Watchdog operational mode.
+ */
+#define WDT_OP_MODE_RESET		(0)
+#define WDT_OP_MODE_INTERRUPT_RESET	BIT(0)


A better name would be WDT_MODE_CALLBACK. The callback typically will, but actually does not have to trigger a reset.

@mnkp Thanks, I will improve that.

mnkp · 2017-08-28T12:57:52Z

include/watchdog.h

+ */
+#define WDT_NO_PAUSE_HALTED_BY_DBG	(0)
+#define WDT_PAUSE_HALTED_BY_DBG		BIT(2)
+#define WDT_PAUSE_HALTED_BY_DBG_MASK	(1 << 2)


In Zephyr's API we typically provide a bit field option to change API's default behavior. I.e. The only required define here is WDT_PAUSE_HALTED_BY_DBG. The other two WDT_NO_PAUSE_HALTED_BY_DBG and WDT_PAUSE_HALTED_BY_DBG_MASK can be removed. This will make it consistent with the rest of the API. Doxygen comment should be updated accordingly.

mnkp · 2017-08-28T13:01:44Z

include/watchdog.h

+ * timeouts - pointer to an array storing timeouts values in microseconds
+ *	      for all enabled channels. If watchdog supports multiple channels
+ *	      but only one counter reload value then the first value from the
+ *	      array will be taken.


Kernel timer API is using milliseconds so indeed we would be more consistent sticking with this unit. Also watchdogs often work with slow / imprecise clock, microsecond resolution is not required.

mnkp · 2017-08-28T13:33:05Z

include/watchdog.h

-	enum wdt_mode mode;
-	void (*interrupt_fn)(struct device *dev);
+	u32_t *timeouts;
+	void (*interrupt_fn)(struct device *dev, u8_t channel);


We could provide a typedef for this function as in

/** Watchdog callback */ typedef void (*wdt_callback_t)(struct device *dev, u8_t channel);

this is sometimes required / useful when writing a driver.

mnkp · 2017-08-28T13:46:51Z

include/watchdog.h

 };

-typedef void (*wdt_api_enable)(struct device *dev);
-typedef void (*wdt_api_disable)(struct device *dev);
+typedef int (*wdt_api_enable)(struct device *dev);


SPI API does not but most of the others Zephyr's APIs add '_t' at the end of the function name in typedef. This seems reasonable. Another clean and used practice is to skip typedef completely and place definitions directly in the struct as in:

struct wdt_driver_api { int (*enable)(struct device *dev); ... };

In this case typedef is actually not required/useful.

Actually there is no strict rules with adding a '_t' or not, or declaring the functions signatures into the struct directly.
Up to you.

As for the typedef: driver APIs are indeed inconsistent but Zephyr Kernel API is not. It is always using '_t' at the end of the function name in typedef.

mnkp · 2017-08-28T14:17:20Z

include/watchdog.h

-static inline void wdt_enable(struct device *dev)
+/*
+ * @brief Enable watchdog instance.
+ *


Description of function parameters is missing, add them with @ param. Here and in all the remaining functions.

mnkp · 2017-08-28T14:28:08Z

include/watchdog.h

- * WDT configuration struct.
+ * @brief Watchdog configuration struct.
+ *
+ * timeouts - pointer to an array storing timeouts values in microseconds


From the description of 'timeouts' it's not clear what is the length of the array. I think we should mention explicitly that it's num_of_channels field.

mnkp · 2017-08-28T14:39:56Z

include/watchdog.h

+/**
+ * @brief Get configuration of watchdog.
+ */
+static inline void wdt_get_config(struct device *dev,


I wonder if we need this function at all. It forces the device driver to store internally the whole struct wdt_config. Also it is not clear what should be done with config.timeouts array. In struct wdt_config we store only the pointer. It does not have to point to a static variable.

Indeed, not sure this one is useful, it comes from legacy API after all. What would be the use case for this function?

tbursztyka · 2017-08-28T15:18:10Z

include/watchdog.h

+ */
+#define WDT_NO_PAUSE_HALTED_BY_DBG	(0)
+#define WDT_PAUSE_HALTED_BY_DBG		BIT(2)
+#define WDT_PAUSE_HALTED_BY_DBG_MASK	(1 << 2)


1 << 2 is what BIT(2) does. I guess you want to change that and use BIT_MASK() macro? See misc/util.h

tbursztyka · 2017-08-28T15:22:01Z

include/watchdog.h

-	void (*interrupt_fn)(struct device *dev);
+	u32_t *timeouts;
+	void (*interrupt_fn)(struct device *dev, u8_t channel);
+	u8_t operation;


I agree, operation makes sense in SPI due to SPI wording (that field in SPI is affecting how the controller works then).

tbursztyka · 2017-08-28T15:25:17Z

include/watchdog.h

+ * @retval -EFAULT if watchdog instance has not been configured yet.
+ * @retval -EBUSY if watchdog instance has been already enabled.
+ */
+static inline int wdt_enable(struct device *dev)
 {


isn't wdt_enable/wdt_disable something related to PM? (actual implementation in wdt_qmsi seems to ack that - qmsi, against which the legacy wdt api was designed from.)

Looks like you can remove these 2 functions, up to PM to handle that.

Very good point. I believe we need to remove these functions. However wdt_configure function should provide means for disabling the watchdog. While Nordic watchdogs need to be explicitly started in case of other vendors watchdog will typically be active after reset.

Not all wdt controller seem to be possible to disable once started, at least not through the controller itself. And disabling through wdt_configure would look weird.

Maybe we need these 2 after all, and the PM function would redirect to them if enabled.

Considering the fact that we don't need to pass any parameters when we want to disable the watchdog it may be indeed a cleaner / more user friendly design if we have a dedicated wdt_disable function rather than passing some flag to wdt_configure. But we likely won't need wdt_enable.

tbursztyka · 2017-08-28T15:27:03Z

include/watchdog.h

+/**
+ * @brief Get configuration of watchdog.
+ */
+static inline void wdt_get_config(struct device *dev,


Indeed, not sure this one is useful, it comes from legacy API after all. What would be the use case for this function?

tbursztyka · 2017-08-28T15:28:38Z

include/watchdog.h

-	u32_t timeout;
-	enum wdt_mode mode;
-	void (*interrupt_fn)(struct device *dev);
+	u32_t *timeouts;


timeouts with an 's'? [edit] Missed the comment detailing this attribute...

tbursztyka · 2017-08-28T15:33:34Z

include/watchdog.h

+	u32_t *timeouts;
+	void (*interrupt_fn)(struct device *dev, u8_t channel);
+	u8_t operation;
+	u8_t num_of_channels;


The pb with channels here means the user has to know how many channel the targeted hardware can handle: 1, 4, ... ? From that: it will be an issue is the code using wdt API is meant to be portable.

Imo, it should be transparent, and fully handled in the driver:

if the hw supports only 1 config it's easy

if the hw supports n channels (usually not a huge number of these, right? something like 4, 8...), the driver would maintain an array of pointers of wdt_config of channels element. And use an empty "slot" in this array to follow a call of wdt_set_config().

-> in both case, is a subsequent wdt_set_config() cannot be applied do to lack of channels, it would just return -EBUSY.

the config that would trigger the wdt interruption, could be passe in the callback (the relevant wdt_config pointer, that is), same in wdt_reload.

This would hide fully this channel issue.

Now, you may want a function to uninstall a configuration already in use. I guess it depends on the controller (some might not be reconfigured? then it would return -ENOTSUPP)

tbursztyka · 2017-08-28T15:34:51Z

include/watchdog.h

+ *		    pointer is NULL.
+ */
+static inline int wdt_set_config(struct device *dev,
+				 struct wdt_config *config)


btw, why not renaming that one to wdt_configure()? That's a taste issue though.

tbursztyka · 2017-08-28T15:38:09Z

include/watchdog.h

 };

-typedef void (*wdt_api_enable)(struct device *dev);
-typedef void (*wdt_api_disable)(struct device *dev);
+typedef int (*wdt_api_enable)(struct device *dev);


Actually there is no strict rules with adding a '_t' or not, or declaring the functions signatures into the struct directly.
Up to you.

lpereira · 2017-08-30T00:07:44Z

Thanks for taking a look at the API. The cycles-vs-timeout thing is really welcome.

It would be nice to set and get values that are retained during a reset caused by the watchdog. Some devices support this (Quark and ESP32 AFAIK). This is useful specially for testing purposes.

The ESP32 watchdog offers a way to determine the boot reason, as well, but I'm not sure if this fits in a generic watchdog interface.

tbursztyka

I like that version

tbursztyka · 2017-09-04T15:45:09Z

include/watchdog.h

+	wdt_window window;
+	void *handle;
+	wdt_callback_t callback;
+	struct wdt_timeout_cfg *next;


nice addition

Nice indeed :)

tbursztyka · 2017-09-04T15:46:03Z

include/watchdog.h

-	void (*interrupt_fn)(struct device *dev);
+struct wdt_timeout_cfg {
+	wdt_window window;
+	void *handle;


Is this handle filled in by the caller or by the driver? (maybe a little bit more doc about it to clarify would be needed then)

@tbursztyka It is handle filled by the caller. As there was no agreement what should be the handle I have left that decision to user. It can point to thread id, config structure or whatever.
You are right the documentation needs improvement.

Ah, if that is handled by the user then maybe it's not needed. The user owns the wdt_timeout_cfg pointer: it could be used as the handle then. Could save a pointer of memory.
(and it lets the possibility for the user to use that pointer in a smart way through CONTAINER_OF() and then access some private data of its own)

@tbursztyka using pointer to wdt_timeout_cfg as a handle is a really bad idea. As I argued before we don't save anything, in contrary we waste sizeof(wdt_timeout_cfg) RAM for every stage or every channel. It's better if wdt_timeout_cfg variable lives shortly on a stack when we call wdt_install_timeout() function. No need to force it to be static. Not mentioning the fact that having pointer to wdt_timeout_cfg as a handle would further complicate this API. It's already pretty complex.

True, missed that point.

@mnkp Having all wdt_timeout_cfg as const and stored in flash would not waste RAM, isn't it?
I am just asking, I am also not a big fan of storing wdt_timtout_cfg pointer as handle.

Having all wdt_timeout_cfg as const and stored in flash would not waste RAM, isn't it?

That's correct.

tbursztyka · 2017-09-04T15:49:42Z

include/watchdog.h

+ * @retval -ENOMEM if no more timeouts can be installed.
+ * @retval -EINVAL if any of the window timeout value is out of possible range.
+ * @retval -ENOEXEC if WDT_MODE_CALLBACK is set but interrupt function
+ *		    pointer is NULL.


I don't know it should be a terminal error here. Isn't it so the nrf5 wdt for instance can be set to trigger an interrupt on underflow/error (which could call a cb then) but will still reset anyway?

Maybe just a SYS_LOG_WARN() in this case would be just fine (to tell the dev that maybe he is doing something unwanted)

nvlsianpu · 2017-09-05T08:44:24Z

include/watchdog.h

-	enum wdt_mode mode;
-	void (*interrupt_fn)(struct device *dev);
+struct wdt_timeout_cfg {
+	wdt_window window;


Documentation: I think It is worth to mention that In case a device doesn't support the window feature the window.down value is ignored or (better) must be equal to 0. It should be told unambiguously.

mnkp

Good work! I do have some extra comments though.

mnkp · 2017-09-05T10:26:11Z

include/watchdog.h

+ */
+struct wdt_window {
+	u16_t up;
+	u16_t down;


Let's change the field names to: min, max. When we are talking about timeout window having the minimum and maximum timeout value is more straightforward; up, down is confusing.

mnkp · 2017-09-05T10:34:21Z

include/watchdog.h

- * WDT configuration struct.
+ * @brief Watchdog timeout configuration struct.
+ *
+ * @param window - timing parameters of watchdog timeout.


This should be changed to

@param window timing parameters of watchdog timeout.

i.e. we should get rid of '-'. Here and in a few other places. It will not look good when rendered by Doxygen.

mnkp · 2017-09-05T10:39:12Z

include/watchdog.h

+ */
+struct wdt_window {
+	u16_t up;
+	u16_t down;


We should use the same type as Kernel API, that is s32_t to keep APIs consistent. Also u16_t is limited to 65.5 seconds, for some applications that may be too short.

@mnkp Could you explain why signed is used in kernel API. Using signed here, where negative values makes no sense is at least weird for me.

Good question. I'm afraid I cannot give any sensible answer here. Maybe someone else?

isn't it so because there is no garanty the timer will exactly be stopped at the right tick, so the remaining time might be below 0 (which then is a stop condition for the timer as well as if it would be 0)

mnkp · 2017-09-05T11:16:00Z

include/watchdog.h

-	u32_t timeout;
-	enum wdt_mode mode;
-	void (*interrupt_fn)(struct device *dev);
+struct wdt_timeout_cfg {


This structure is missing a way to specify timeout action for multistage watchdogs. I.e. we should be able to support the following scenarios.

Scenario 1:

stage 1 -> run callback_1

stage 2 -> run callback_2

stage 3 -> perform SoC reset

Scenario 2:

stage 1 -> run callback_1

stage 2 -> run callback_2

stage 3 -> run callback_3

We may also want to ensure that it's possible to extend this API in the future (if it ever becomes a requirement) to support Scenario 3:

stage 1 -> run callback_1

stage 2 -> run callback_2

stage 3 -> perform CPU core reset

stage 4 -> perform SoC reset

It seems like WDT_MODE_CALLBACK should belong to this struct and not to wdt_cfg. Also maybe it would make sense to replace WDT_MODE_CALLBACK with WDT_MODE_SOC_RESET? If we provide a valid (not NULL) callback pointer it means we want the watchdog driver to run the callback (no need to re-confirm this by explicit WDT_MODE_CALLBACK) and we provide WDT_MODE_SOC_RESET flag if we want a reset to be performed. We would support this way all different watchdog hardware:

Atmels SAM family - single channel, single stage watchdog. If callback is configured the reset will not be performed.

NXP K64 family - single channel, single stage watchdog. If callback is configured it will be executed, reset will always be performed 256 bus clock cycles later.

Espressif ESP32 family - single channel, multiple stage watchdog. Every stage can be independently configured to either execute a callback or trigger a reset.

Nordic nRF51 series - multi-channel, single stage watchdog. If callback is configured it will be executed, reset will always be performed 2 slow clock cycles later.

E.g. if we try to configure NXP K64 watchdog without providing WDT_MODE_SOC_RESET flag the call will return ENOTSUP.

We may also choose not to use WDT_MODE_SOC_RESET flag and rely on the default SoC behavior. In this case however user application will not be able to determine if the watchdog will or will not perform a reset when it configures a callback. To get around this we may provide mechanism to retrieve watchdog capabilities.

@mnkp I was also thinking about that. The question is if that API should be so robust or maybe we should left configuring the behavior of each stage in Kconfig.SoC. Suppose that WDT_MODE_CALLBACK is not set so there generally will be some kind of reset.
What kind of reset and after what stage could be configured in Kconfig. Then driver would only check if configuring timeouts is consistent with options set in Kconfig. For example if following option is desired:

stage 1 -> run callback_1

stage 2 -> run callback_2

stage 3 -> perform SoC reset

the driver would return some error code if not enough wdt_timeout_cfg are supplied.

There are a few problems with using Kconfig:

It will make the API opaque. User will have difficulty determining what is happening.

it will not be very portable. Either every driver will implement their own set of Kconfig options or we try to come up with a common set of options defined in drivers/watchdog/Kconfig. The former is not portable, in the latter case we won't be able to map the options between different driver instances. Unless we put it in DTS but it's not straightforward. I think this should be considered the last resort.

mnkp · 2017-09-05T12:53:41Z

include/watchdog.h

-	void (*interrupt_fn)(struct device *dev);
+struct wdt_timeout_cfg {
+	wdt_window window;
+	void *handle;


@tbursztyka using pointer to wdt_timeout_cfg as a handle is a really bad idea. As I argued before we don't save anything, in contrary we waste sizeof(wdt_timeout_cfg) RAM for every stage or every channel. It's better if wdt_timeout_cfg variable lives shortly on a stack when we call wdt_install_timeout() function. No need to force it to be static. Not mentioning the fact that having pointer to wdt_timeout_cfg as a handle would further complicate this API. It's already pretty complex.

mnkp · 2017-09-05T13:18:39Z

include/watchdog.h

+ * @retval -EFAULT if watchdog instance has not been configured yet.
+ * @retval -EBUSY if watchdog instance has been already started.
+ */
+static inline int wdt_start(struct device *dev)


This function is Nordic nRF51 specific. Typically a watchdog is started/stopped when it is configured. Also virtually all watchdogs are started upon reset so having an explicit wdt_start function is confusing. Watchdogs can only be reconfigured. The function can be removed.

hum depends actually. On some SoC you can really stop wdt running via clock controller, and make it running again the same way. That does not seem nrf specific to me.

K64 subfamily can be explicitly stopped through the WDOG_EN bit in the watchdog status and control register. nRF5 family can be explicitly started but can't be explicitly stopped. I would left both wdt_start and wdt_stop and return -ENOTSUP if SoC does not support called functionality.

@tbursztyka, @m-kru you are both right and I understand the reasoning behind having the two functions but I still would like to argue that we are better without them.

There are some watchdogs which can be stopped and then restarted but this is a niche behavior. If we get rid of wdt_start we will still support watchdogs which allow to be stopped and restarted with wdt_stop, wdt_configure combo. Rather than having

wdt_stop() wdt_install_timeout() ... wdt_install_timeout() wdt_configure() wdt_start()

we would have

wdt_stop() wdt_install_timeout() ... wdt_install_timeout() wdt_configure()

wdt_configure() and wdt_start() would have to be used in pairs anyway.

When writing an application we generally shouldn't concern ourselves with the exact driver implementation but the way API was designed. So we would need to use wdt_configure() and wdt_start() also for watchdogs that can't be re-started.

To sum up:

We don't loose any functionality if we remove this function.

For majority of the users familiar with the way their watchdog works the API will be more straightforward to use.

@mnkp Would wdt_configure() automatically start watchdog or should it be configurable (I think it should automatically start it as there is rather no sense in configuring watchdog and not enabling it)?

Btw. I have realized that in API RFC there is no word about uninstalling timeouts. Should we provide such functionality via separate function, or maybe wdt_stop() should automatically clear all installed timeouts in addition to stopping watchdog? (If yes I think it should be renamed to wdt_disable()).

wdt_configure() does not start the watchdog, it installs the controller's "behavior".

about loosing installed timeouts, I guess it depends on controllers again.

We need only one function to configure and start the watchdog. Maybe we can come up with a different name? The only alternative that comes to my mind is wdt_control. Anything else?

wdt_setup() ?
@tbursztyka @mnkp Is there any agreement if there should be one or two functions for configuring and starting watchdog, cause I am a bit lost with that issue right now?

well, maybe mixing up controller's config and starting is a good idea. wdt_start() was not bad, just remove wdt_config and put the parameter into wdt_start?

wdt_start() is not a good name for this function as most of the watchdog modules will already be running when the function is called. User seeing wdt_start() function and not calling it will be right to assume that the watchdog will stay in the disabled state. That's incorrect.

I think wdt_setup() is a really good name as it reflects well what the function is doing.

mnkp · 2017-09-05T13:27:54Z

include/watchdog.h

+/**
+ * @brief Pause watchdog timer when CPU is halted by the debugger.
+ */
+#define WDT_PAUSE_HALTED_BY_DBG		BIT(2)


Currently this API has only one field, namely 'options' which requires bit-field defines. But this may change in the future in case the API is extended. E.g. we still need to provide a function to determine the boot reason as requested by @lpereira. To avoid confusion I propose to proceed above defines with WDT_OPT_ to make clear they refer to the options field. It's a common practice and it will make it consistent with I2S API. I.e. WDT_PAUSE_HALTED_BY_DBG should become WDT_OPT_PAUSE_HALTED_BY_DBG

mnkp · 2017-09-05T19:18:38Z

I have one more comment. Currently watchdog API is missing from the official Zephyr API Documentation. I think this PR should also update one of the files in doc/api/. I'm however not sure what's the best section to put it.

We should also run doxygen to verify that all doc strings are rendered as expected. I couldn't find the official documentation explaining how it should be done. As far as I remember one has to run

make -C doc htmldocs

from Zephyr's home directory. There may be some dependencies which need to be installed.

andrewboie · 2017-09-05T19:20:37Z

$ pip3 install --user -r scripts/requirements.txt

that will install all deps needed to build documentation

andyross

Style & API design whinery.

andyross · 2017-09-05T19:35:12Z

include/watchdog.h

+/**
+ * @brief Pause watchdog timer when CPU is in sleep state.
+ */
+#define WDT_PAUSE_IN_SLEEP	BIT(1)


Devil's advocacy: why is this an option? Under what circumstance would you want a driver to fail to automatically pause a watchdog during sleep?

Maybe what we really want is a flag set by the driver that tells the user "watchdog does NOT automatically pause for sleep, you have to do it yourself!" ?

@andyross I am sorry, I am not sure what you mean, since when driver controls the user not the other way.
It is straightforward I guess, you either want watchdog counter to stop counting when CPU is in sleep state or you want it to keep counting, it depends on the application.

andyross · 2017-09-05T19:39:12Z

include/watchdog.h

-	WDT_2_30_CYCLES,
-	WDT_2_31_CYCLES
+struct wdt_cfg {
+	u8_t options;


Sorry, I don't get this one. Why is this a struct without named fields? If you want a simple way to pass three flags, then just pass a flags word directly and not a pointer to a configuration struct. If you're going to go through the trouble to make a struct, we should at least get typesafety out of the deal with e.g. single-bit fields named "callback_en", "pause_in_sleep" and "pause_in_debug" or whatever.

Thanks, I am aware of that, I will improve it in next iteration.

andyross · 2017-09-05T19:43:06Z

include/watchdog.h

+ *		    installed so far.
+ */
+static inline int wdt_install_timeout(struct device *dev,
+				      struct wdt_timeout_cfg *cfg)


Similar whine: wdt_timeout_cfg has only two fields. Forcing the user to fill them out on the stack and pass a pointer isn't actually saving anything (in fact I'd expect code size would be rather larger) vs. just passing them as "min" and "max" arguments to the function right here. Seems needlessly complicated to me.

@andyross wdt_timeout_cfg has 4 fields (or 5 I do not know how to count structure inside structure).

andrewboie · 2017-09-05T19:55:19Z

include/watchdog.h

+
+/*
+ * This is the default, and will be the way until the new API below
+ * will be enforced everywhere.


What exactly is the plan for this?
We have been carrying around "spi" and "spi_legacy" for quite some time, and the legacy API is still the default..

@andrewboie Well, I guess the reason is that no one feels responsible for updating the drivers.

@andrewboie Well, I guess the reason is that no one feels responsible for updating the drivers.

When changing API changes we can't expect drivers to be changed over-night to an API that is still in review. If we just change the APIs to support new IPs and h without looking at the existing drivers we might end up with drivers that can't be moved to the new API easily, looking at existing drivers should be part of the change, or there should be at least a plan.

For starters, we need to have a list of issues added for each driver that needs to be changed, preferably with some instructions for driver owner explaining what changed and what to look for when moving to the new API.

Do we really have to create this _legacy header? Can't we change the API inline deprecating the old APIs? I know it was done for SPI, does not mean it is needed for every API change we do? Maybe it is easier with _legacy.h when implementing the API, but adds lots of complexity and confusion and old API are kept alive and are not deprecated proper.

Can't we change the API inline deprecating the old APIs?

@nashif
You mean making one watchdog.h file and adding deprecated suffix/prefix to old stuff?

yes, that has been the right way to change APIs. Not sure if that is possible at all, but at least it gives us an easier migration path without introducing the LEGACY craft.

__deprecated should not fail the build, this was changed some time ago.

re naming, yes, that will be tricky. Right now however moving everything to be legacy, i.e. renaming driver names to _legacy and declaring this as the new API without having a single driver use it is not helping. Without actually looking at the existing drivers and providing example of how to migrate and working with the driver owners to move to the new API this will be a problem.
We do not want to end up with an API that favours one HW and ignores the others, and that was the case we had initially given that the API was put together to address the first WD driver we introduced to Zephyr.

I agree, a driver implementing this new API in the same PR would be nice.
Renaming existing drivers with _legacy is not needed, only the dependency in Kconfig is.

In case of spi, responding to @andrewboie, the legacy is the default because of qmsi and the fact that the related hw was our reference platform. I guess, knowing these hw fate now, we can make the new API the default. That said, even new platform like nrf5 is still made on top of old API.
We need to push driver maintainers to go with new APIs asap.

I'm also much in favor of using __deprecated rather than going the LEGACY way. The former is cleaner, with __deprecated user receives very precise warnings at compile time pointing out things that need to be changed.

That said, wdt_reload() is indeed a problem as we've changed the function prototype. We would have to come up with a new name. The two quite popular among different watchdog driver implementations are wdt_kick() and wdt_feed().

not as good as wdt_reload() but well, if we need a new, let's have a new name.

wdt_feed() is better. We do not want to get sued by defenders of animal rights.

dbkinder · 2017-09-05T20:58:17Z

https://github.com/zephyrproject-rtos/zephyr/blob/master/doc/README.rst tell how to setup and build documentation

m-kru

Hope we are close to the finish with this API.

m-kru · 2017-09-11T12:46:33Z

include/watchdog.h

+ * timeouts then min value must be equal to 0.
+ *
+ * @param min Lower limit of watchdog reload timeout in microseconds.
+ * @param max Upper limit of watchdog reload timeout in microseconds.


I have come back to microseconds and 32 bit values. The question is if there is any need to return actual set timeout values. For example the resolution of watchdog timer is 60 us and super supply 100 us. I think there is no need because users usually estimate timeout values and this estimation is rather coarse. Maybe we should only add information to the documentation that these values are always rounded up and they may vary depending on platform?

As I stated before I am in favor of using the same time unit as Zephyr kernel does. So here is one vote against the change to microseconds. I'll shortly remind my reasoning:

Watchdog clocks are designed to be reliable not precise. Typically these are a very imprecise RC clock sources, often as a fallback when a crystal fails. But even if we had a 32768 crystal oscillator only microsecond precision is not required. We won't use watchdog to measure time. Resolution of watchdog is strictly connected with the system tick. It will never be smaller than a tick length. Watchdog timeout value lays typically between ~10ms to some seconds. It should be as small as possible but has to include all the uncertainties: inaccuracy of the source clock, scheduler. There is always safety margin added to the timeout value and it will never lay in microseconds range. If Zephyr allowed to set system tick to value smaller than 1 ms then the watchdog should do it too but it the same way kernel API does. Let's keep APIs consistent.

The question is if there is any need to return actual set timeout values.

For now we should be fine without providing this information. If there is a need in the future the watchdog API can be extended with a new function. We could also encourage developers to add SYS_LOG_INF in wdt_setup function to print out timing information.

Maybe we should only add information to the documentation that these values are always rounded up

+1

SYS_LOG_INFO with real timeout is great idea - as it is indeed debug related functionality. So I would be up to the will of MCU ports maintainers.

Is adding a @note to the documentation of wdt_setup that it is welcomed to print actual timeouts using SYS_LOG_INFO an abuse?

m-kru · 2017-09-11T12:48:26Z

include/watchdog.h

+/** CPU core and peripherals reset */
+#define WDT_FLAG_RESET_CPU_PER	BIT(2)
+/** Global system reset */
+#define WDT_FLAG_RESET_SOC	(BIT(2) | BIT(1))


I do not know how to make clean group doxygen comment for these macros. I have tried different ways from doxygen manual however none on them worked with our auto generated html documentation.

OK. I found a few issues here that will take a few commits to make work. The way we were including the auto-generated docs used a :content-only: directive that on the surface looks OK, has the side-effect of causing all the doxygen headings to be removed and the API material listed in alphabetic order (instead of separating the functions from the typedefs, for example). Removing the :content-only: fixed that, but resulted in some extra headings and layout issues (that required a CSS tweak). When all this gets merged, you can write this section like this to make it come out as a "group doxygen comment" (note the addition of the @name and the start/stop group markers @{ and @}:

/** * @name Watchdog Reset Behavior * If none of the reset flags is specified there will be no reset after timeout. * @{ */ /** CPU core reset */ #define WDT_FLAG_RESET_CPU>-----BIT(1) /** CPU core and peripherals reset */ #define WDT_FLAG_RESET_CPU_PER>-BIT(2) /** Global system reset */ #define WDT_FLAG_RESET_SOC>-----(BIT(2) | BIT(1)) /** @} */

Ready to use this documentation grouping syntax now.

mnkp

We are certainly close to finish with this API. It's time to drop RFC from the commit title.

mnkp · 2017-09-12T07:55:09Z

include/watchdog.h

+
+/*
+ * This is the default, and will be the way until the new API below
+ * will be enforced everywhere.


I'm also much in favor of using __deprecated rather than going the LEGACY way. The former is cleaner, with __deprecated user receives very precise warnings at compile time pointing out things that need to be changed.

That said, wdt_reload() is indeed a problem as we've changed the function prototype. We would have to come up with a new name. The two quite popular among different watchdog driver implementations are wdt_kick() and wdt_feed().

mnkp · 2017-09-12T08:08:31Z

include/watchdog.h

+ * @brief Run callback function on watchdog timeout. If any of reset flags is
+ *	  set this callback is called before reset.
+ */
+#define WDT_FLAG_RUN_CALLBACK	BIT(0)


Now that we have WDT_FLAG_RESET_* flags this one could be removed. We would only need to mention in the description of the callback parameter that passing NULL means that no callback will be run.

If WDT_FLAG_RUN_CALLBACK flag stays the driver will have to check if it is consistent with the value of the callback parameter and return an error if not. That's an extra code. By removing the flag we reduce code size and simplify the API.

mnkp · 2017-09-12T08:26:12Z

include/watchdog.h

+/** CPU core and peripherals reset */
+#define WDT_FLAG_RESET_CPU_PER	BIT(2)
+/** Global system reset */
+#define WDT_FLAG_RESET_SOC	(BIT(2) | BIT(1))


I believe we need the following two defines only:

#define WDT_FLAG_RESET_CPU BIT(1) #define WDT_FLAG_RESET_SOC BIT(2)

We probably should be more explicit and rename the first one to WDT_FLAG_RESET_CPU_CORE.

Also, as I mentioned until we re-write our device drivers to take into account the reset source users should not really use WDT_FLAG_RESET_CPU_CORE as it is not supported. So for now we could leave it out completely. This flag can be added later when there is a project which needs it and comes up with a sensible device driver design that supports the flag.

mnkp · 2017-09-12T08:48:19Z

include/watchdog.h

+ * timeouts then min value must be equal to 0.
+ *
+ * @param min Lower limit of watchdog reload timeout in microseconds.
+ * @param max Upper limit of watchdog reload timeout in microseconds.


As I stated before I am in favor of using the same time unit as Zephyr kernel does. So here is one vote against the change to microseconds. I'll shortly remind my reasoning:

Watchdog clocks are designed to be reliable not precise. Typically these are a very imprecise RC clock sources, often as a fallback when a crystal fails. But even if we had a 32768 crystal oscillator only microsecond precision is not required. We won't use watchdog to measure time. Resolution of watchdog is strictly connected with the system tick. It will never be smaller than a tick length. Watchdog timeout value lays typically between ~10ms to some seconds. It should be as small as possible but has to include all the uncertainties: inaccuracy of the source clock, scheduler. There is always safety margin added to the timeout value and it will never lay in microseconds range. If Zephyr allowed to set system tick to value smaller than 1 ms then the watchdog should do it too but it the same way kernel API does. Let's keep APIs consistent.

mnkp · 2017-09-12T08:59:03Z

include/watchdog.h

+ * timeouts then min value must be equal to 0.
+ *
+ * @param min Lower limit of watchdog reload timeout in microseconds.
+ * @param max Upper limit of watchdog reload timeout in microseconds.


The question is if there is any need to return actual set timeout values.

For now we should be fine without providing this information. If there is a need in the future the watchdog API can be extended with a new function. We could also encourage developers to add SYS_LOG_INF in wdt_setup function to print out timing information.

Maybe we should only add information to the documentation that these values are always rounded up

+1

mnkp · 2017-09-12T09:23:04Z

include/watchdog.h

+ * @retval -ENOTSUP If any of the set options is not supported.
+ * @retval -EBUSY If watchdog instance has been already setup.
+ */
+static inline int wdt_set_up(struct device *dev, u8_t options)


This looks like a correct English grammar but set-up function has always been called setup. Maybe we should stick with the convention and change it to wdt_setup?

From a grammar point of view, "setup" is a noun (or adjective) while "set up" is a verb (phrase), so you can "set up a chess board", or you can "go to a setup screen", or you can "pay a setup fee" (generally not used with a hyphen.) Alas, @mnkp is probably right with the convention of using "setup" rather than "set_up" in function names even if it's an action.

mnkp · 2017-09-12T09:38:09Z

include/watchdog.h

-				 struct wdt_config *config)
+/**
+ * @brief Install new timeout.
+ *


Watchdogs are typically started on reset, many can be configured only once. This means that there can only be one function which can perform actual hardware changes and it has to be wdt_setup(). We should state here that wdt_install_timeout() will not perform any actual changes, not until wdt_setup() is called (even if description of wdt_setup() already suggests such behavior).

mnkp · 2017-09-12T09:46:04Z

include/watchdog.h

+ * @brief Disable watchdog instance.
+ *
+ * This function stops watchdog instance and automatically uninstalls all
+ * timeouts. To set up watchdog again it is required to install timeouts.


* timeouts. * * To set up watchdog again it is required to install timeouts and call * wdt_setup function. Not all watchdogs can be re-started once they are * disabled.

mnkp · 2017-09-12T09:53:43Z

include/watchdog.h

+ * @retval -EBUSY If timeout can not be installed while watchdog has already
+ *		  been setup.
+ * @retval -ENOMEM If no more timeouts can be installed.
+ * @retval -EFAULT If timeout with given handle has already been installed.


We should allow using the same handle for multistage watchdogs. Only for multi-channel watchdogs it would be a problem.

@mnkp I have added following sentence to wdt_timeout_cfg structure documentation.

It is allowed to use the same handle for multistage timeouts.

Is it enough or do you want to mention about that also in the documentation of wdt_install_timeout() function?

It's probably best not to repeat the information twice but I think that the -EFAULT error description should mention that it applies to multi-channel watchdogs only.

Sorry, I did not consider this case carefully enough. For the most popular single channel, single stage watchdogs we also shouldn't allow to re-install the timeout (since we don't allow to re-install timeouts for multi-channel watchdogs). So the sentence should better be changed to something like "Multistage watchdogs are allowed to use the same handle for different stages" or "This limitation does not apply to multistage watchdogs".

mnkp · 2017-09-12T10:13:42Z

drivers/watchdog/Kconfig

+	  2 WARNING, write SYS_LOG_WRN in addition to previous level
+	  3 INFO, write SYS_LOG_INF in addition to previous levels
+	  4 DEBUG, write SYS_LOG_DBG in addition to previous levels
+


I propose to add WDT_DISABLE_AT_BOOT option to Kconfig to allow disabling watchdog at boot. Useful at the initial stage of the project development as well as for many sample applications where we don't explicitly handle watchdogs.

@mnkp Should it depend on WATCHDOG? It would be funny if you enable WATCHDOG to disable it :).

True, but actually it should depend on WATCHDOG. If the watchdog subsystem is not enabled we can't disable it.

@mnkp You think it should be default y or default n?
By the way which one is correct: at boot or on boot?

I think it should be default n. The board defconfig file can still choose to set it to default y if deemed required.

Later we should only ensure that all the applications in samples/ directory that don't explicitly handle watchdog have WDT_DISABLE_AT_BOOT option enabled.

dbkinder · 2017-09-12T22:28:20Z

doc/api/timer_counter_interfaces.rst

+
+.. doxygengroup:: watchdog_interface
+   :project: Zephyr
+   :content-only:


delete this line (:contents-only:)

dbkinder · 2017-09-12T22:32:37Z

include/watchdog.h

+#define WDT_FLAG_RUN_CALLBACK	BIT(0)
+
+/**
+ * @brief Watchdog reset behavior.


recommend using the grouping syntax noted below; the doc infrastructure needed has been merged now. As this is written these comments only apply to the first define (WDT_FLAG_RESET_CPU)

dbkinder

a few recommended edits

mnkp

@dbkinder parts of the documentation were re-written, it would be great if you could double check the style. We're not native English speakers so you are the only hope.

mnkp · 2017-10-04T17:48:09Z

include/watchdog.h

 {
 	const struct wdt_driver_api *api = dev->driver_api;

 	api->enable(dev);
 }

-static inline void wdt_disable(struct device *dev)
+static inline void __deprecated wdt_disable(struct device *dev)


@m-kru did you try to compile this file? I believe it would result in an error as wdt_disable function is defined twice. To solve this issue it should be enough to remove the second definition, lines 269-274. struct wdt_driver_api also needs an update.

@mnkp This is the case where we want to update API and use the same function name as in the old one. Such case has been already discussed here but without any conclusion, maybe @nashif could say how it should be solved?

In both new and old API wdt_disable function has the same functionality and the same prototype (almost). The only difference is the function return value: int in the new API, void in the old one.

I propose to keep wdt_disable as it is. You'll need to remove the second definition, lines 269-274 and additionally modify all existing watchdog drivers to ensure wdt_disable returns int. I believe this is the easiest solution. Unless I overlooked something?

Your current proposal is invalid, it does not parse in compiler as reported by Shippable. There are two more compile time errors which need to be fixed.

@mnkp I am aware of that, I just was not sure how to solve this function name issue. I will update PR today.

mnkp · 2017-10-04T18:18:56Z

include/watchdog.h

+ * @retval -EBUSY If timeout can not be installed while watchdog has already
+ *		  been setup.
+ * @retval -ENOMEM If no more timeouts can be installed.
+ * @retval -EFAULT If timeout with given handle has already been installed.


Sorry, I did not consider this case carefully enough. For the most popular single channel, single stage watchdogs we also shouldn't allow to re-install the timeout (since we don't allow to re-install timeouts for multi-channel watchdogs). So the sentence should better be changed to something like "Multistage watchdogs are allowed to use the same handle for different stages" or "This limitation does not apply to multistage watchdogs".

mnkp · 2017-10-04T18:25:00Z

include/watchdog.h

+/**
+ * @brief Disable watchdog instance.
+ *
+ * This function stops watchdog instance and automatically uninstalls all


Maybe we should write "This function disables watchdog instance"? In this case user will not have to consider if there is any difference between stopping and disabling the watchdog.

agree! But use:

This function disables the watchdog instance...

mnkp · 2017-10-04T18:27:34Z

include/watchdog.h

+ * @param dev Pointer to the device structure for the driver instance.
+ * @param options Bit field with following parts:
+ *
+ *     pause in sleep      [ 0 ]       - pause in CPU sleep


"- pause when CPU is in sleep state" ?

Or we could simply say:

@param options Configuration options as defined by the WDT_OPT_* constants.

mnkp · 2017-10-04T19:21:40Z

include/watchdog.h

+ */
+/** CPU core reset */
+#define WDT_FLAG_RESET_CPU_CORE		BIT(0)
+/** Global system reset */


I would write "Global SoC reset" or "Global chip reset". System reset suggests that also other board peripherals (on the PCB, external to SoC) will be reset. However this is SoC/board specific, doesn't have to be the case.

mnkp · 2017-10-04T19:36:10Z

include/watchdog.h

+/**
+ * @brief Watchdog reset flag bit field mask.
+ */
+#define WDT_FLAG_RESET_MASK	(0x3)


Since we encode every reset type as a separate flag/bit (we use BIT macro) I think we shouldn't define WDT_FLAG_RESET_MASK. However, if we do want to treat WDT_FLAG_RESET_* as a multi-bit bit field we should define it as follows:

#define WDT_FLAG_RESET_SHIFT (0) #define WDT_FLAG_RESET_MASK (0x3 << WDT_FLAG_RESET_SHIFT) #define WDT_FLAG_RESET_NONE (0 << WDT_FLAG_RESET_SHIFT) #define WDT_FLAG_RESET_CPU_CORE (1 << WDT_FLAG_RESET_SHIFT) #define WDT_FLAG_RESET_SOC (2 << WDT_FLAG_RESET_SHIFT)

to be consistent with remaining Zephys APIs.

mnkp · 2017-10-04T19:57:54Z

include/watchdog.h

+ * @note If specified values can not be precisely set they are always
+ *	 rounded up.
+ */
+struct wdt_window {


There was a suggestion before to flatten this whole structure directly into struct wdt_timeout_cfg to make it easier for the user to define timeout configuration. That may be not a bad idea. Did we make a final decision? Anyone against/for such a change?

mnkp · 2017-10-04T20:12:58Z

include/watchdog.h

+ *
+ * This function is used for configuring global watchdog settings that
+ * affect all timeouts. It should be called after installing timeouts.
+ * After successful return all installed timeouts are valid.


are valid and have to be serviced periodically by calling wdt_feed function.

mnkp · 2017-10-04T20:19:55Z

include/watchdog.h

+}
+
+/**
+ * @brief Reload specified watchdog timeout.


"Feed specified watchdog timeout." would sound strange, but using 'reload' interchangeably with 'feed' may also be non obvious. Wouldn't 'service' be a better choice. As in "Service specified watchdog timeout."?

But the function name is wdt_feed so "feed" is consistent (and appears in many other places)

So let's stay with "feed". Thanks @dbkinder for the review!

mnkp · 2017-10-04T20:22:01Z

include/watchdog.h

+ * @brief Reload specified watchdog timeout.
+ *
+ * @param dev Pointer to the device structure for the driver instance.
+ * @param handle Handle of timeout that should be fed.


And this could be changed to "Handle of timeout that should be serviced."

Don't want to "service" sometimes and "feed" others, so given the name of the function is "feed", I'd think you want to keep to using "feed".

Thanks for clarification. While "feeding the watchdog" sounds natural I was not sure if "feeding the watchdog timeout" is also OK, i.e. I was not sure if "feed" can be used in a context it was used here.

dbkinder · 2017-10-04T22:06:48Z

include/watchdog.h

+/**
+ * @brief Watchdog timeout window.
+ *
+ * Each installed timeout needs feeding in specified time window, otherwise


better would read:

... needs feeding within the specified ...

nashif · 2017-10-10T21:36:03Z

include/watchdog.h

@@ -1,23 +1,39 @@
 /*
- * Copyright (c) 2015 Intel Corporation.
+ * Copyright (c) 2017 Nordic Semiconductor ASA


You should not remove existing copyrights, just add the new copyright on top please.

durub

Minor English nitpicks

durub · 2017-10-10T21:21:04Z

include/watchdog.h

+ * @param dev Pointer to the device structure for the driver instance.
+ * @param options Configuration options as defined by the WDT_OPT_* constants
+ *
+ * @retval 0 If success.


English nitpick: should be "If successful".

durub · 2017-10-10T21:21:36Z

include/watchdog.h

+ *
+ * @param dev Pointer to the device structure for the driver instance.
+ *
+ * @retval 0 If success.


Same here ("if successful").

durub · 2017-10-10T21:21:41Z

include/watchdog.h

+ * @param dev Pointer to the device structure for the driver instance.
+ * @param cfg Pointer to timeout configuration structure.
+ *
+ * @retval 0 If success.


Same here ("if successful").

durub · 2017-10-10T21:46:27Z

include/watchdog.h

+ * @param dev Pointer to the device structure for the driver instance.
+ * @param handle Handle of timeout that should be fed.
+ *
+ * @retval 0 If success.


Same here ("if successful").

m-kru · 2017-10-11T11:18:42Z

@nashif @mnkp @durub I have applied your concerns.

mnkp

There is one unaddressed comment from @dbkinder but otherwise it LGTM. Thanks @m-kru for working on a new API!

nashif · 2017-10-11T13:07:04Z

do not rename the current test (wdt_deprecated), leave it as is and it should be fixed to use the new APIs and probably made more generic, right now it is very qmsi specific.

New API enables setting watchdog timeout in the unit of microseconds. It is possible to configure watchdog timer behavior in CPU sleep state as well as when it is halted by the debugger. It supports watchdogs with multiple reload channels. Jira: ZEP-2564 Signed-off-by: Michał Kruszewski <[email protected]>

galak · 2017-11-09T14:39:05Z

At this point, I feel like this is now 1.11, any concerns related to pushing to 1.11?

mnkp · 2017-11-13T14:55:29Z

This PR is all but ready. As far as I can tell all issues have already been addressed, only not yet approved by respective reviewers. It would be great if everyone followed up on the issues they reported. The status of the PR would be clearer.

That said it probably doesn't make sense to merge this PR in 1.10 if there are no drivers which implement it. If no one reports new issues we should merge this PR as soon as the window for 1.11 release is open. This will give us enough time to update the watchdog drivers. Or should the process be different?

There is also a question on how we should split the work. Shall we create an issue for every watchdog driver that needs to be converted to the new API so any developer interested in working on the task can take ownership of it?

…ct-rtos#1286) This includes base configuration common to all platforms. Fixes zephyrproject-rtos#1260. Signed-off-by: Geoff Gustafson <[email protected]>

mnkp · 2018-01-06T20:27:23Z

This PR is all but ready, shouldn't we merge it? It will give us some time to update watchdog drivers before 1.11 release window is closed. It would be shame to leave it hanging after so much discussion and time invested.

nashif · 2018-01-06T20:42:36Z

This PR is all but ready, shouldn't we merge it?

yes, agree. There are still a few unaddressed comments, but those are minor IMO. Will put this on the top of the list for next week.

anangl · 2018-01-25T14:19:35Z

include/watchdog.h

+ *		   window differs from windows for alarms installed so far.
+ */
+static inline int wdt_install_timeout(struct device *dev,
+				      struct wdt_timeout_cfg *cfg)


Is there a reason why the second parameter is not a pointer to a constant structure? It seems that this function is not supposed to modify anything in the pointed content. And currently one cannot use a constant structure (e.g. stored in flash) without doing ugly typecasting.

Good point. It certainly makes sense to make it const. We've overlooked it.

nvlsianpu · 2018-01-26T13:04:18Z

drivers/watchdog/Kconfig

+	  3 INFO, write SYS_LOG_INF in addition to previous levels
+	  4 DEBUG, write SYS_LOG_DBG in addition to previous levels
+
+config HAVE_WDT_MULTISTAGE


i there any SoC which has it already?

ESP32 supports 4 watchdog stages.

pfalcon · 2018-03-02T14:29:50Z

Looks like this is not getting into 1.11 (it's in pre-release freeze already). Changing milestone to 1.12.

nashif · 2018-05-18T20:17:22Z

this was superseded by another PR

m-kru requested a review from nashif as a code owner August 25, 2017 14:48

m-kru commented Aug 25, 2017

View reviewed changes

m-kru changed the title ~~RFC:API: Update Watchdog API~~ drivers: watchdog: Watchdog API redesign [RFC] Aug 25, 2017

nvlsianpu requested changes Aug 28, 2017

View reviewed changes

mnkp requested changes Aug 28, 2017

View reviewed changes

tbursztyka requested changes Aug 28, 2017

View reviewed changes

galak added this to the v1.10 milestone Aug 30, 2017

tbursztyka reviewed Sep 4, 2017

View reviewed changes

nvlsianpu reviewed Sep 5, 2017

View reviewed changes

mnkp requested changes Sep 5, 2017

View reviewed changes

andyross reviewed Sep 5, 2017

View reviewed changes

andrewboie reviewed Sep 5, 2017

View reviewed changes

m-kru requested a review from dbkinder as a code owner September 11, 2017 12:38

ghost assigned m-kru Sep 11, 2017

ghost added the In progress For PRs: is work in progress and should not be merged yet. For issues: Is being worked on label Sep 11, 2017

m-kru commented Sep 11, 2017

View reviewed changes

dbkinder mentioned this pull request Sep 12, 2017

doc: groups of items in API documentation not displaying #1445

Closed

mnkp requested changes Sep 12, 2017

View reviewed changes

dbkinder reviewed Sep 12, 2017

View reviewed changes

dbkinder suggested changes Sep 12, 2017

View reviewed changes

nashif modified the milestones: v1.10, v1.10.0 Oct 3, 2017

tbursztyka previously approved these changes Oct 4, 2017

View reviewed changes

mnkp reviewed Oct 4, 2017

View reviewed changes

dbkinder reviewed Oct 4, 2017

View reviewed changes

nashif requested changes Oct 10, 2017

View reviewed changes

durub suggested changes Oct 10, 2017

View reviewed changes

mnkp approved these changes Oct 11, 2017

View reviewed changes

lpereira mentioned this pull request Oct 18, 2017

tests: watchdog: Added support for esp32 #1287

Closed

galak modified the milestones: v1.10.0, v1.11.0 Nov 14, 2017

nashif added RFC Request For Comments: want input from the community and removed RFC Request For Comments: want input from the community labels Nov 18, 2017

mnkp mentioned this pull request Nov 22, 2017

watchdog: document the watchdog API and constants. #5083

Closed

nashif added the area: API Changes to public APIs label Jan 6, 2018

anangl reviewed Jan 25, 2018

View reviewed changes

nvlsianpu approved these changes Jan 26, 2018

View reviewed changes

galak mentioned this pull request Jan 30, 2018

Driver API review/cleanup/rework #5697

Closed

33 tasks

anangl mentioned this pull request Feb 13, 2018

watchdog: adding driver for the nrf52 family #6047

Closed

pfalcon modified the milestones: v1.11.0, v1.12.0 Mar 2, 2018

kl-cruz mentioned this pull request Apr 10, 2018

drivers: wdt: Watchdog API redesign and shim for nrfx wdt driver #7007

Merged

nashif closed this May 18, 2018

ghost removed the In progress For PRs: is work in progress and should not be merged yet. For issues: Is being worked on label May 18, 2018

drivers: watchdog: Watchdog API redesign [RFC] #1260

drivers: watchdog: Watchdog API redesign [RFC] #1260

Conversation

m-kru commented Aug 25, 2017 • edited by nashif Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbursztyka Aug 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnkp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-kru Aug 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-kru Aug 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbursztyka Aug 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpereira commented Aug 30, 2017

tbursztyka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnkp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-kru Sep 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnkp Sep 5, 2017 • edited Loading

Choose a reason for hiding this comment

m-kru commented Aug 25, 2017 •

edited by nashif

Loading

tbursztyka Aug 28, 2017 •

edited

Loading

m-kru Aug 28, 2017 •

edited

Loading

m-kru Aug 28, 2017 •

edited

Loading

tbursztyka Aug 28, 2017 •

edited

Loading

m-kru Sep 5, 2017 •

edited

Loading

mnkp Sep 5, 2017 •

edited

Loading