drivers: counter: add Maxim DS3231 support #17631

pabigot · 2019-07-17T22:28:10Z

This PR provides support for the Maxim DS3231 TCXO RTC, a high-precision real-time clock peripheral with support for battery backup.

This is implemented as a counter, but the DS3231 has significant functionality enhancements beyond a general RTC API. Also, as an I2C peripheral it has limitations that are incompatible with assumptions about how a counter can function. Most users of its advanced functionality will interact with it through device specific API provided in a driver-specific header following the approach summarized in #17072. Consequently the interface file is placed in the (existing) include/drivers/rtc directory, and the extension functions are named based on it being a specialization of a (no longer existing) rtc driver class.

There is a question of where to collect the documentation. Because it's device specific, it really should be its own page at API Reference : Peripherals : RTC : DS3231. For now I've just put it under (newly created) RTC.

As more infrastructure is added to support management of civil time tracking in Zephyr some of the functionality in this driver may need to be generalized. For example, the rtc_ds3231_syncpoint structure should probably be generalized and placed in the recently created <sys/timeutil.h> header long with functions that support generic conversion from a synchronization point and local time to RTC and back.

zephyrbot · 2019-07-17T22:34:43Z

All checks are passing now.

Review history of this comment for details about previous failed status.
Note that some checks might have not completed yet.

doc/reference/peripherals/rtc.rst

samples/drivers/rtc_ds3231/README.rst

dbkinder

Sigh. OK for now. I'll add checking for overviews for the API docs to the bin list :)

dbkinder

The doc build is failing because there's no visible option for CONFIG_APP_SET_ALIGNED_CLOCK defined in the Kconfig tree?

samples/drivers/rtc_ds3231/README.rst

drivers/counter/ds3231.c

dbkinder

+1 for doc changes, thanks.

carlescufi · 2019-07-25T15:38:13Z

@anangl could you please take a look at the proposed API extension mechanism?

pabigot · 2019-07-31T23:35:20Z

Added to dev-review in hopes of getting feedback on this specifically, and device-specific APIs in general. TSC mention and posting on devel didn't produce much/anything. Base question: can this make it into 2.0.0?

drivers/counter/ds3231.c

pabigot · 2020-04-25T12:51:08Z

This has been rebased and updated with proper locking and usermode support for the extended sync functions and the underlying counter API. I would like to see something like the rtc_ds3231_{req,get}_syncpoint() API in whatever we use for RTC extensions.

Please provide feedback, particularly @jdascenzio @henrikbrixandersen @sslupsky who have expressed interest in RTC. I would like to get this in to 2.3, rather than attempt to beat @nordic-krch 's record of 367 days from PR open to merge.

jfischer-no · 2020-04-28T16:18:24Z

drivers/counter/ds3231.c

+
+	/* Flags recording requests for ISW monitoring. */
+	u8_t isw_mon_req;
+#define ISW_MON_REQ_Alarm 0x01


That is unusual.

You mean mixed case? I suppose, though it's standard for some roles (e.g. CMSIS-style bitfield values), and is also present internally Zephyr's time_utils.h where the units suffix is (correctly) in lower case.

If it really bothers people I'll uglify it.

I meant to define a macro in middle of a struct.

carlescufi · 2020-04-28T16:29:36Z

@henrikbrixandersen and @jdascenzio could you please review, especially in the context of the work for #23526

The DS3231 is an I2C real-time clock with internal temperature compensated oscillator, maintaining civil time to 1 s precision with nominal 2 ppm accuracy from 0-40 Cel. The basic functionality is exposed as a counter that is always running at 1 Hz. Much more functionality is exposed as driver-specific API, including the ability to translate between the time scale of the DS3231 and the time scale of the Zephyr uptime clock. This allows correlation of events in the system clock to UTC, TAI, or whatever time scale is used to maintain the DS3231. Signed-off-by: Peter A. Bigot <pab@pabigot.com>

Cloned from counter_basic_api with modifications based on DS3231 limitations: * Only one device tested per board; * Counter cannot be stopped or started; * Alarms are serviced by worker thread, so are not invoked from ISR and require that test thread yield to allow processing (no k_busy_wait()); * Multiple Alarms test is disabled as documented in test. Additional tests were added for DS3231-specific API. Signed-off-by: Peter A. Bigot <pab@pabigot.com>

pabigot · 2020-04-30T13:54:02Z

Rebased and all public symbols renamed from rtc_ds3231_ to maxim_ds3231_ in anticipation of possible dedicated RTC API.

jfischer-no · 2020-05-05T08:28:06Z

include/drivers/rtc/maxim_ds3231.h

+ * @param user_data the corresponding parameter from
+ * maxim_ds3231_alarm::user_data.
+ */
+typedef void (*maxim_ds3231_alarm_callback_handler_t)(struct device *dev,


maxim_ds3231_alarm_callback_handler_t is to long, please use shorter names, here ds3231_alarm_cb_handler is fine.

Overall just ds3231/DS3231 instead of maxim_ds3231/MAXIM_DS3231 would be sufficient.

Why not generally abbreviate callback with cb if the prefix give the only feasible interpretation like ds3231_notify_cb?

While I started with ds3231_ as the prefix, and that has prior art in tree for (e.g.) ccs811_, Zephyr has not carefully managed namespaces and I don't want to assume that there will never be conflicts between device model numbers across vendors. Standardizing on the compatible name makes sense. The negative impact is small, only affecting those who are using the custom API.

This proposed policy is something we should discuss more widely. So: #24978

callback is unambiguous while cb is only clear in context with interpretation. IMO the biggest problem with long-but-clear names is a consequence of 8-space indentation with a 78 character line limit. I have no reason to believe the shorter version ever directly improves comprehension, and some reason to believe it makes it worse (increased cognitive load from requiring an interpretation at all).

I'm seeing 44 foo_callback_t typedefs, and 96 foo_cb_t typedefs, in Zephyr today.

Not to say I won't change it if people insist, but that's why I picked the names that I did.

henrikbrixandersen

I have mainly reviewed the DS3231 specific API extensions. Looks good!

I think we should get this merged. Once we have more RTC drivers in-tree, we can look at extending the counter API in a generic fashion or even defining a new, RTC specific API.

henrikbrixandersen · 2020-05-05T13:53:39Z

doc/reference/peripherals/index.rst

   pwm.rst
   ps2.rst
   peci.rst
+   rtc.rst


Should we include this? Or rather wait until we have an API to document?

The documentation for the driver-specific extensions ends up here. It does state that it's a placeholder.

henrikbrixandersen · 2020-05-05T13:59:26Z

samples/drivers/counter/maxim_ds3231/sample.yaml

+tests:
+  sample.basic.maxim_ds3231:
+    build_only: true
+    platform_whitelist: efr32mg_sltb004a frdm_k64f nrf51_pca10028 nucleo_l476rg particle_xenon


Would it make sense to provide a generic DS3231 shield definition utilising standard connectors such as Arduino instead of the many, board specific overlays?

The ChronoDot isn't physically compatible with Arduino headers, and at least two boards that are supported don't have them either. So I'm reluctant to do that.

I think we should get this merged. Once we have more RTC drivers in-tree, we can look at extending the counter API in a generic fashion or even defining a new, RTC specific API.

There are a few drivers in the tree already. Many of the existing SOC's have RTC devices on-chip. Zephyr is using them for the system timer (as opposed to a counter). So, in this regard, I think it is quite important to advance the discussion of an RTC api before it becomes a terrible mess to fix in the future.

carlescufi · 2020-05-05T16:59:47Z

@nordic-krch would you mind approving this if you have no objections?

carlescufi · 2020-05-07T12:52:14Z

@jfischer-phytec-iot @sslupsky @nordic-krch please approve if you are fine with this PR as-is.

carlescufi · 2020-05-07T17:45:01Z

Merging this since it's been open for far too long. If you have additional comments please voice them to @pabigot and they can go in as bug fixes during the 2.3 stabilization period.

cederom · 2021-05-20T08:16:00Z

This is a nice starting point thank you but also requires external chip. I have created a feature request here: #35333 :-)

pabigot added area: Drivers area: Counter labels Jul 17, 2019

pabigot requested review from MaureenHelm, carlescufi, dbkinder, galak, nashif and nordic-krch as code owners July 17, 2019 22:28

zephyrbot added area: Devicetree area: API Changes to public APIs area: Samples Samples area: Documentation labels Jul 17, 2019

pabigot force-pushed the pr/20190717a branch from decbd1d to 20134d1 Compare July 17, 2019 22:39

dbkinder suggested changes Jul 22, 2019

View reviewed changes

pabigot force-pushed the pr/20190717a branch from 20134d1 to 0caba25 Compare July 22, 2019 22:47

dbkinder approved these changes Jul 22, 2019

View reviewed changes

dbkinder suggested changes Jul 22, 2019

View reviewed changes

samples/drivers/rtc_ds3231/README.rst Outdated Show resolved Hide resolved

pabigot force-pushed the pr/20190717a branch from 0caba25 to 6c3567b Compare July 22, 2019 23:41

nordic-krch reviewed Jul 23, 2019

View reviewed changes

drivers/counter/ds3231.c Outdated Show resolved Hide resolved

dbkinder approved these changes Jul 24, 2019

View reviewed changes

carlescufi requested a review from anangl July 25, 2019 15:35

pabigot mentioned this pull request Jul 28, 2019

drivers/gpio: specify whether driver is ISR-safe #17827

Closed

pabigot force-pushed the pr/20190717a branch from 6c3567b to fe6138f Compare July 30, 2019 18:45

pabigot added the dev-review To be discussed in dev-review meeting label Jul 31, 2019

nordic-krch reviewed Aug 1, 2019

View reviewed changes

drivers/counter/ds3231.c Outdated Show resolved Hide resolved

drivers/counter/ds3231.c Outdated Show resolved Hide resolved

galak added the TSC Topics that need TSC discussion label Aug 1, 2019

pabigot added the DNM This PR should not be merged (Do Not Merge) label Apr 20, 2020

pabigot force-pushed the pr/20190717a branch 2 times, most recently from 4cd6079 to 53e153e Compare April 24, 2020 13:43

zephyrbot added the area: Tests Issues related to a particular existing or missing test label Apr 24, 2020

pabigot force-pushed the pr/20190717a branch 3 times, most recently from 153bdc8 to 0f25c1c Compare April 24, 2020 21:16

pabigot removed the DNM This PR should not be merged (Do Not Merge) label Apr 24, 2020

pabigot force-pushed the pr/20190717a branch from f892ef1 to 38669ba Compare April 25, 2020 10:58

pabigot added this to the v2.3.0 milestone Apr 25, 2020

pabigot requested review from henrikbrixandersen and removed request for pizi-nordic April 25, 2020 12:51

pabigot mentioned this pull request Apr 27, 2020

lib: drivers: clock: rtc: rtc api to maintain calendar time through reboot #24625

Closed

jfischer-no reviewed Apr 28, 2020

View reviewed changes

pabigot force-pushed the pr/20190717a branch from 954caa6 to 38669ba Compare April 29, 2020 13:14

pabigot added 2 commits April 30, 2020 08:53

pabigot force-pushed the pr/20190717a branch from 38669ba to f1f9494 Compare April 30, 2020 13:53

jfischer-no reviewed May 5, 2020

View reviewed changes

pabigot mentioned this pull request May 5, 2020

RFC: use compatible name for prefix for device-specific API #24978

Closed

henrikbrixandersen approved these changes May 5, 2020

View reviewed changes

carlescufi merged commit e697656 into zephyrproject-rtos:master May 7, 2020

pabigot deleted the pr/20190717a branch May 13, 2020 18:51

drivers: counter: add Maxim DS3231 support #17631

drivers: counter: add Maxim DS3231 support #17631

Uh oh!

Conversation

pabigot commented Jul 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zephyrbot commented Jul 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dbkinder left a comment

Choose a reason for hiding this comment

Uh oh!

dbkinder left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dbkinder left a comment

Choose a reason for hiding this comment

Uh oh!

carlescufi commented Jul 25, 2019

Uh oh!

pabigot commented Jul 31, 2019

Uh oh!

Uh oh!

Uh oh!

pabigot commented Apr 25, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlescufi commented Apr 28, 2020

Uh oh!

pabigot commented Apr 30, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

henrikbrixandersen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlescufi commented May 5, 2020

Uh oh!

carlescufi commented May 7, 2020

Uh oh!

carlescufi commented May 7, 2020

Uh oh!

cederom commented May 20, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

pabigot commented Jul 17, 2019 •

edited

Loading

zephyrbot commented Jul 17, 2019 •

edited

Loading

dbkinder left a comment •

edited

Loading