drivers: rtc: define RTC API driver #41287

nfaurant · 2021-12-17T08:16:57Z

define RTC api in rtc.h
implement rtc driver using internal stm32u5 rtc
add rtc sample to test internal stm32u5

erwango

Thanks for this proposal.

Please the change into several commits:

API
Driver implementation
Sample

Then, I guess we should go into api meeting to see if the proposed api can make it in.
@carlescufi ?

- Currently all RTC peripherals are implemented through Counter - This new API is specific to RTC independently from Counter Signed-off-by: FAURANT Nicolas <[email protected]>

- this illustrates RTC API proposal - this driver use stm32 hal API Signed-off-by: FAURANT Nicolas <[email protected]>

- set time - get time - wake up timer interrupt - AlarmA and AlarmB interrupts Signed-off-by: FAURANT Nicolas <[email protected]>

erwango · 2021-12-20T16:43:22Z

As a reference for further discussion, previous attempt to introduce RTC API: #23526

carlescufi · 2021-12-23T11:09:04Z

Thanks for this proposal.

Please the change into several commits:

API

Driver implementation

Sample

Then, I guess we should go into api meeting to see if the proposed api can make it in. @carlescufi ?

Indeed. @nfaurant thank you for raising this subject again. I would recommend that you read through the conclusions that we wrote down the last time we brought this up in the API meeting:
#23526 (comment)

And compare the original PR with yours. After that, if you think it's suitable then we can bring it to the meeting again.

nfaurant · 2021-12-23T14:02:24Z

And compare the original PR with yours. After that, if you think it's suitable then we can bring it to the meeting again

The purpose of this new API specific to an RTC driver, is to dissociate the use of an RTC features of the Counter API driver.

Indeed, a use case can require the use of the following 2 functionnalities:

counting external pulses (via the Counter driver)
having an RTC time base, and wake up periodically or on programmed alarms (via the RTC driver)

Hence, the interest to separate funcionnalities in 2 drivers

github-actions · 2022-02-22T00:29:08Z

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

de-nordic · 2022-03-18T10:29:28Z

include/drivers/rtc.h

+	int (*set_wakeup_timer)(const struct device *dev, struct rtc_wakeup wut, enum rtc_wakeup_id wut_id);
+	int (*start_wakeup_timer)(const struct device *dev, enum rtc_wakeup_id wut_id);
+	int (*stop_wakeup_timer)(const struct device *dev, enum rtc_wakeup_id wut_id);


Shouldn't wake up be separate API? Have external i2c/parallel/spi devices been considered?
You may have devices like https://datasheets.maximintegrated.com/en/ds/DS1371.pdf or https://ww1.microchip.com/downloads/en/DeviceDoc/MCP7940M-Low-Cost%20I2C-RTCC-with-SRAM-20002292C.pdf, connected via i2c, that may use the alarm API but does not have wake up functionality. On the other side it is possible, on some devices, to use watchdog as a wake up device, even though the device itself does not have RTC, only various timers.
Having "wake-up" functionality as mandatory means that a lot of i2c devices will just have -ENOTSUP added just because it is required.

Some external RTC (like NXP PCF85063A) use this feature under the name "Countdown timer" or "programmable timer".
"wakeup timer" is the name used by ST.
I suggest keeping this feature in the RTC API because this timer is specific to the RTC and sometimes it'usefull for ultra low power periodic wake-up. The user is free to use them or not. In this way the API is generic.
Like for the other driver, if an RTC doesn't implement the wakeup timer fonctionnality, the driver can return -ENOTSUP with a log message:
LOG_ERR("Wake up timer is not supported");
return -ENOTSUP;

(...) In this way the API is generic. Like for the other driver, if an RTC doesn't implement the wakeup timer fonctionnality, the driver can return -ENOTSUP with a log message: LOG_ERR("Wake up timer is not supported"); return -ENOTSUP;

Problem with generic APIs that they bring a lot of stuff that you may not be using but will take up your flash anyway.
Separate wake-API would be more generic because it would allow to not only cover this specific RTC tied feature but the mentioned watchdog based or count down based, whatever it is called, when you are not interested in entire RTC thing.

We can add a symbol in the Kconfig of the rtc

config RTC_EN_WUT
bool "Enable the rtc wake up timer support"
help
This option enables the rtc wake up timer API calls.

And decide to enable or not the wake up timer API
__subsystem struct rtc_driver_api {
int (*set_time)(const struct device *dev, const struct tm date_time);
int (*get_time)(const struct device *dev, struct tm *date_time);
#ifdef CONFIG_RTC_EN_WUT
int (*set_wakeup_timer)(const struct device *dev, struct rtc_wakeup wut, enum rtc_wakeup_id wut_id);
int (*start_wakeup_timer)(const struct device *dev, enum rtc_wakeup_id wut_id);
int (*stop_wakeup_timer)(const struct device *dev, enum rtc_wakeup_id wut_id);
#endif
int (*set_alarm)(const struct device *dev, struct rtc_alarm alarm, enum rtc_alarm_id alarm_id);
int (*start_alarm)(const struct device *dev, enum rtc_alarm_id alarm_id);
int (*stop_alarm)(const struct device *dev, enum rtc_alarm_id alarm_id);
};
idem for the wut data ...

In this way, an rtc can implement the wut or not without additional memory constraints ...

I am not sure whether #ifdefing API interface is good idea, I know it is done at least with Flash API, but I remember that there have been some arguments against this approach.

Anyway, your proposal is actually reverse of I was thinking of.
I will describe thing I did some time ago to better let you understand what I have meant:
the device I have in mind was controlling several other devices and conducting certain tests on them (that could take several minutes) at specified intervals and then would go to sleep; the device had been sleeping most of the time, when not doing its main job, to save power as it could be running on batteries (VRLA, AGM or GEL, on a power failure), and would be woken up by a watchdog at certain intervals (ranging from minutes to hours) depending on previous state. The second wake up channel, external, would be a communication device that would wake up the control device upon receiving request from base.
So both ways to get the device to life would be via wake up channel, one timed and one asynchronous interrupt, in both cases the device would check comm device for any messages to be processed.
As you can see such device would have to get entire API to just use the wake up portion of it.

I agree with @de-nordic that this seems better as a separate API to implement in addition to the RTC API, rather than lumped in here with an #ifdef. It feels like it's polluting the core idea of what it means to be an RTC with a (common but specific) implementation type.

Therefore, those 3 functions should be removed form rtc API

thedjnK · 2022-05-14T13:30:57Z

@nfaurant Any resolution for the conflict?

NickolasLapp · 2022-05-20T19:32:35Z

include/drivers/rtc.h

+	RTC_ALARM_WEEKDAY_SEL,  /**< Alarm Weekday is selected, set parameter to a value in the 0-6 range (days since Sunday). */
+};
+
+/** @brief Specifies the RTC Alarm Masks. */


A bit more specificity about what this mask means exactly would be useful.
I'm looking at RTC functionality for the MAX31343 as a possible use case for this API.

That RTC has the option to mask a variety of time fields (from seconds up to years), but only some combinations are valid. For example, you cannot mask to match on minutes but not on seconds. I think that's implied with this enum but I am not sure without more context.

I test this driver with an internal rtc of a STM32 µC (an example was committed).
For this rtc you can add a mask on

secondes => RTC_ALARM_MASK_SEC

minutes => RTC_ALARM_MASK_MIN

hours => RTC_ALARM_MASK_HOURS

date or weekday => RTC_ALARM_MASK_DATE_WEEKDAY

If you select RTC_ALARM_MASK_MIN the alarm will occur every minutes
if you select RTC_ALARM_MASK_DATE_WEEKDAY the alarm will occur

every sunday with alarm_time.tm_wday = 0 and use alarm_date_weekday_sel=RTC_ALARM_WEEKDAY_SEL

every monday with alarm_time.tm_wday = 1 and use alarm_date_weekday_sel=RTC_ALARM_WEEKDAY_SEL
...

every 2nd of month with alarm_time.tm_mday = 2 and use alarm_date_weekday_sel=RTC_ALARM_DATE_SEL

bjarki-andreasen · 2022-06-21T17:42:00Z

include/drivers/rtc.h

+/** @brief Wake up timer structure. */
+struct rtc_wakeup {
+	uint32_t period;                                /**< Wake up timer period in milliseconds. */
+	void (*callback)(const struct device *dev);     /**< Callback function. Can be NULL. */


We should provide a user_data variable with the alarm callback to allow for multiple instances of drivers, or C++ classes, which will share the same static callback for different alarms. They can provide pointers to themselves in the user_data field.

void (*callback)(const struct device *dev, void *user_data); /**< Callback function. Can be NULL. */

For reference this is from the counter API

struct counter_top_cfg { uint32_t ticks; counter_top_callback_t callback; void *user_data; uint32_t flags; };

We should also add a boolean to choose between periodic wakeup timer and singleshot wakeup timer

bool periodic; /**< Set if wakeup timer should be periodic*/

Or the inverse if the default is periodic

bool singleshot; /**< Set if wakeup timer should be single shot*/

bjarki-andreasen · 2022-06-21T17:48:02Z

include/drivers/rtc.h

+	 * If WeekDay date is selected, use tm_wday [0 to 6] to set the alarm
+	 */
+	enum rtc_alarm_date_weekday alarm_date_weekday_sel;
+	void (*callback)(const struct device *dev);     /**< Callback function. Can be NULL. */


See the first paragraph of this comment https://github.com/zephyrproject-rtos/zephyr/pull/41287/files#r902898525

bjarki-andreasen · 2022-06-21T18:16:56Z

I don't think there will ever be more than one wakeup timer in an RTC, just more alarms, since any counter capable of causing an interrupt, can be used as a "wakeup timer" during runtime, the only thing it does different is running when the device is powered down... I don't think there needs to be an id for the wakeup timer for this reason, so we could simplify the code by removing the timer id enum.

Using enums to specify alarm id is a too rigid approach i would say, as it will require updating the API every time a "better" RTC device (with more alarms) is supported, and it can easily lead to issues where two parts of an application are setting the same alarm by accident, which is a bug that can be hard to track down.

Instead, an id (or handle), could be returned when using the alarm_set function, this will alert the application if it tries to set more timers/alarms than the RTC supports, and will prevent two+ parts of the application from using the same alarm/timer.

This would require a new function like "alarm_give" or similar to make it available again, but remove the need for the enums, making it support "infinite" alarms/triggers from the start in a safer manner.

A compromise would be to add a similar function as the one implemented in the counter API, where you have a uint8_t channel argument, which can be probed by using the counter_get_num_of_channels(), which is not safer, but more flexible.

This branch contains our proposal to what we would like the RTC API to look like, this is useful comparison and discussion:
bjarki-andreasen/zephyr@main...bjarki-trackunit:add_rtc_driver_api

I also think we should move away from using the struct tm structure as it is 36 bytes which can be optimized, the tm_day for example is stored as an int... I created a datetime structure which is more appropriate for low ram devices

cfriedt · 2022-07-21T15:42:08Z

include/drivers/rtc.h

+/** @brief Wake up timers enum. */
+enum rtc_wakeup_id {
+	RTC_WUT0,       /**< Wake Up timer 0. */
+	/* Add new item before RTC_WUT_NUM. */


This doesn't scale very well for devices that support more than 1 wakeup id. It should not be required to add enumerations to a global header just to index a wakeup id.

I would suggest just using size_t.

Out of curiosity, have you ever seen a RTC with more than 1 wakeup timer? Seems unnecessary since only the wakeup timer with the shortest period has any influence on wakeup. Since the RTC must be ultra low power, adding another one would just increase power consumption without adding meaningful functionality.

I would remove the ID, unless anyone knows of an RTC with more than one wakeup timer :)

http://ww1.microchip.com/downloads/en/devicedoc/20005010f.pdf

Dual Programmable Alarms

The alarms are different from wakeup timers, most RTCs have more than one alarm, but only one wakeup timer if any. The Wakeup timer just counts up to a match value, and then invokes an interrupt, which can be mapped to wake up the device. The point of the timer is to be very low power, and as such, only has this limited capability.
Alarms on the other hand use the date and time to invoke alarms, and require much more power.

The device linked here has to alarms, no wakeup timer http://ww1.microchip.com/downloads/en/devicedoc/20005010f.pdf

Alternatively, just use DT chosen, and then the ID is implicit.

cfriedt · 2022-07-21T15:43:38Z

include/drivers/rtc.h

+enum rtc_alarm_id {
+	RTC_ALARMA,     /**< Alarm A. */
+	RTC_ALARMB,     /**< Alarm B. */
+	/* Add new item before RTC_ALARM_NUM. */


Same as above - couldn't we simply use size_t?

Better than size_t, we should just use DT chosen and a device handle.

github-actions · 2022-09-20T00:33:41Z

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

cfriedt · 2022-11-01T15:51:03Z

@stephanosio - this is the most recent attempt to define an RTC API, FWIR

cfriedt · 2022-11-01T15:51:28Z

@nfaurant - this may become relevant shortly.

yonsch · 2022-11-28T14:50:47Z

Any updates? This would be useful for the RP2040

nordicjm · 2022-11-28T14:52:36Z

Any updates? This would be useful for the RP2040

At this point I think someone else needs to take this PR and finish/submit it.

bjarki-andreasen · 2022-11-28T15:57:15Z

@yonsch @nordicjm We have a complimentary PR I wrote a while ago #46747, I will adapt it to match the comments and suggestions of this PR and create a new PR for it. I will link it here when ready, we can determine how to move forward from there :) Probably 1-2 days before it is ready

cfriedt · 2022-11-28T23:53:42Z

@bjarki-trackunit - I just made some additional comments.

It would really make more sense to pass around a device handle rather than using integer IDs for wakeup or alarm devices. DT chosen can then be used to make selection uniform across systems.

bjarki-andreasen · 2022-11-29T07:34:35Z

@bjarki-trackunit - I just made some additional comments.

It would really make more sense to pass around a device handle rather than using integer IDs for wakeup or alarm devices. DT chosen can then be used to make selection uniform across systems.

I agree, maybe something like this?

chosen {
    zephyr,wakeup-alarm = <&alarma>;
};

rtcc: rtcc@4202000 {
    alarma: alarm@4202010 {
    };
    alarmb: alarm@4202020 {
    };
};

de-nordic · 2022-11-29T08:36:26Z

@bjarki-trackunit @cfriedt I think that we need RFC PR with collected notes and comments from existing approaches to the RTC API.

nordicjm · 2022-11-29T08:43:47Z

Note that this specific PR was the one that was discussed and accepted at the API working group a long time ago.

carlescufi · 2022-11-29T15:58:51Z

@nfaurant is this something you'd like to continue working on, or would you rather someone else take over?

github-actions · 2023-01-29T00:31:06Z

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

BenBenSS · 2023-08-10T11:56:53Z

Hi, Sir. I am using the HighTEC compiler and using TC275 to control MCP7940M, From the oscilloscope, it can be seen that the slave address was written twice, followed by the data written (0x12, 0x55, 0x08). Why is the slave address written twice? The correct ones should be the slave address, slave register address, and data. Did I configure them incorrectly? Please Help me.

bjarki-andreasen · 2023-08-10T12:12:02Z

@BenBenSS Hi, I would recommend you use the official Discord server to get help with device driver issues :)

nfaurant requested review from ABOSTM, carlescufi, erwango, FRASTM, galak, MaureenHelm and nashif as code owners December 17, 2021 08:16

github-actions bot added area: API Changes to public APIs area: Devicetree area: Documentation area: Samples Samples platform: STM32 ST Micro STM32 labels Dec 17, 2021

erwango added the RFC Request For Comments: want input from the community label Dec 17, 2021

erwango requested changes Dec 17, 2021

View reviewed changes

nfaurant added 3 commits December 17, 2021 10:02

drivers: rtc: Dedicated API proposal

b3399ee

- Currently all RTC peripherals are implemented through Counter - This new API is specific to RTC independently from Counter Signed-off-by: FAURANT Nicolas <[email protected]>

drivers: rtc: Driver implementation for stm32u5

9e7504b

- this illustrates RTC API proposal - this driver use stm32 hal API Signed-off-by: FAURANT Nicolas <[email protected]>

drivers: rtc: sample to test internal stm32u5 rtc

1579882

- set time - get time - wake up timer interrupt - AlarmA and AlarmB interrupts Signed-off-by: FAURANT Nicolas <[email protected]>

nfaurant force-pushed the rtc_driver branch from 002427c to 1579882 Compare December 17, 2021 09:07

erwango mentioned this pull request Dec 20, 2021

stm32: counter: making counter date-time friendly #41318

Closed

zephyrbot added the area: Build System label Dec 21, 2021

zephyrbot requested review from gmarull, mbolivar-nordic, SebastianBoe, tejlmand and utzig December 21, 2021 13:55

zephyrbot assigned erwango Dec 21, 2021

de-nordic reviewed Mar 18, 2022

View reviewed changes

NickolasLapp reviewed May 20, 2022

View reviewed changes

bjarki-andreasen reviewed Jun 21, 2022

View reviewed changes

cfriedt reviewed Jul 21, 2022

View reviewed changes

github-actions bot added the Stale label Sep 20, 2022

github-actions bot closed this Oct 4, 2022

cfriedt reopened this Nov 1, 2022

cfriedt removed the Stale label Nov 1, 2022

nordicjm mentioned this pull request Nov 17, 2022

Raspberry Pi Pico RTC Driver #52312

Closed

carlescufi mentioned this pull request Nov 29, 2022

include/drivers: Add RTC support #52618

Merged

github-actions bot added the Stale label Jan 29, 2023

github-actions bot closed this Feb 13, 2023

drivers: rtc: define RTC API driver #41287

drivers: rtc: define RTC API driver #41287

Conversation

nfaurant commented Dec 17, 2021

erwango left a comment

Choose a reason for hiding this comment

erwango commented Dec 20, 2021

carlescufi commented Dec 23, 2021

nfaurant commented Dec 23, 2021

github-actions bot commented Feb 22, 2022

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

thedjnK commented May 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjarki-andreasen Jun 21, 2022 • edited Loading

Choose a reason for hiding this comment

bjarki-andreasen Jun 21, 2022 • edited Loading

Choose a reason for hiding this comment

bjarki-andreasen commented Jun 21, 2022 • edited Loading

cfriedt Jul 21, 2022 • 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

cfriedt Nov 28, 2022 • edited Loading

Choose a reason for hiding this comment

cfriedt Jul 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 20, 2022

cfriedt commented Nov 1, 2022

cfriedt commented Nov 1, 2022

yonsch commented Nov 28, 2022

nordicjm commented Nov 28, 2022

bjarki-andreasen commented Nov 28, 2022

cfriedt commented Nov 28, 2022

bjarki-andreasen commented Nov 29, 2022 • edited Loading

de-nordic commented Nov 29, 2022

nordicjm commented Nov 29, 2022

carlescufi commented Nov 29, 2022

github-actions bot commented Jan 29, 2023

BenBenSS commented Aug 10, 2023

bjarki-andreasen commented Aug 10, 2023

bjarki-andreasen Jun 21, 2022 •

edited

Loading

bjarki-andreasen Jun 21, 2022 •

edited

Loading

bjarki-andreasen commented Jun 21, 2022 •

edited

Loading

cfriedt Jul 21, 2022 •

edited

Loading

cfriedt Nov 28, 2022 •

edited

Loading

cfriedt Jul 21, 2022 •

edited

Loading

bjarki-andreasen commented Nov 29, 2022 •

edited

Loading