ds323x: Minimal driver for DS323x extremely accurate RTC #9838

jnohlgard · 2018-08-24T12:54:45Z

Contribution description

A simple device driver for enabling the square wave output on the DS3234 SPI RTC from Maxim.
This driver currently only supports the SPI connected DS3234, and only for initializing 1 Hz square wave output on the SQW pin, nothing else.

The intended use case is to use this for calibrating internal oscillators on different boards by connecting the square wave output to a GPIO interrupt pin, and through those measurements achieve better accuracy from cheaper timers.

The register map is the same for all DS323x, but only the DS3234 is implemented here.

Testing procedure

External hardware: https://www.sparkfun.com/products/10160
Required tools: Oscilloscope or logic analyzer

Configure the device parameters in ds3234_params.h, build and flash tests/driver_ds3234. Examine the SQW pin on the RTC using an oscilloscope or logic analyzer, see a 1 Hz square wave.

Issues/PRs references

nothing yet.

PeterKietzmann

@gebart code-wise I'm not convinced how/why the ds3233 and ds3234are separated like this. Is there reason why some files are common but the params files, headers and the test are not?
Besides that, the code looks good to me. I just ordered a device and can provide test after it arrived.

PeterKietzmann · 2018-08-28T16:29:09Z

drivers/Makefile.dep


+ifneq (,$(filter ds3234,$(USEMODULE)))
+  USEMODULE += ds323x
+  USEMODULE += ds323x_ds3234


This name seems a bit weird to me. Is it even generated or used somewhere?

It's just to get the right file compiled without special tricks to get it recognized as a file from the ds323x directory

PeterKietzmann · 2018-08-28T16:31:23Z

drivers/ds323x/Makefile

@@ -0,0 +1,4 @@
+# enable submodules
+SUBMODULES := 1


So far you just implemented ds3234.c. Do we actually need the SUBMODULES now?

Implemented this way makes it easy to add a ds3231.c later on

PeterKietzmann · 2018-08-28T16:35:46Z

drivers/ds323x/ds3234.c

+        return -EIO;
+    }
+
+    if (ENABLE_DEBUG) {


Why not #ifdef?

To let the compiler syntax check the part even when enable debug is off

PeterKietzmann · 2018-08-28T16:37:05Z

drivers/ds323x/ds3234.c

+    }
+    uint8_t reg = 0;
+    ds3234_read_reg(dev, DS323X_REG_CONTROL, 1, &reg);
+    reg &= ~(DS323X_REG_CONTROL_EOSC_MASK | DS323X_REG_CONTROL_INTCN_MASK |


Would you mind to spend a note which options you (un)set here? Is this a form of device reset?

I'll add a comment. The bits rs1 rs2 select which frequency the sqw pin will output. The intcn bit selects whether the sqw pin is an interrupt output or a square wave signal output. The eosc bit enables the oscillator when it is cleared.

Added a comment

PeterKietzmann · 2018-08-28T16:40:07Z

drivers/ds323x/include/ds3234_params.h

@@ -0,0 +1,65 @@
+/*


Do you think it is necessary to have separate params files?

Since ds3234 is spi connected and the others are i2c, I figured the configuration should be separate

jnohlgard · 2018-08-28T17:24:21Z

Only the ds3234 is spi connected, the others are i2c (ds3231, ds3232). There is no ds3233 AFAIK.
Here's the manufacturer product list
https://para.maximintegrated.com/en/search.mvp?fam=rtc&270=TCXO

PeterKietzmann · 2018-08-28T18:32:58Z

@gebart thanks for the immediate feedback. It wasn't clear to me that the other device is I2C based. Then I'm fine with the file-wise isolation. Let's wait until the device arrives.

jnohlgard · 2018-08-28T18:38:15Z

Sounds good 👍

leandrolanzieri · 2018-09-11T10:13:25Z

I just tested this on a Nucleo F030R8 and on a SAM R21 (after rebasing to apply #9885), and can confirm it works as intended.

Here you can se the 1Hz square wave:

Going through the device datasheet I saw there is no ID register or something like that to evaluate if device is actually present on the bus or not during initialization. Would it be worth it to add a function that writes and reads a byte from the SRAM present on the device? Maybe something like:

bool ds3234_is_present(const ds3234_params_t *dev)
{
    uint8_t value = 0x55, address = 0x0A, reg = 0;
    ds3234_write_reg(dev, DS323X_REG_SRAM_ADDR, 1, &address);
    ds3234_write_reg(dev, DS323X_REG_SRAM_DATA, 1, &value);
    ds3234_write_reg(dev, DS323X_REG_SRAM_ADDR, 1, &address);
    ds3234_read_reg(dev, DS323X_REG_SRAM_DATA, 1, &reg);
    
    if (ENABLE_DEBUG) {
        print_str("Expected: 0x");
        print_byte_hex(value);
        print_str(" - Read: 0x");
        print_byte_hex(reg);
        print_str("\n");
    }

    return value == reg;
}

jnohlgard · 2018-09-11T10:19:23Z

@leandrolanzieri I would prefer to not touch the SRAM portion of the chip. What we could do instead is read back the written register and see that it matches what we expect, if you would like some more feedback to the application.

leandrolanzieri · 2018-09-11T10:28:26Z

@gebart yes, we could do that. The problem is that, with the current configuration, the control register is always 0.

leandrolanzieri · 2018-10-11T14:46:36Z

Maybe we could go without checking if the device is present for now?

PeterKietzmann · 2018-10-15T14:04:08Z

Can't you just write ones to RS1 and RS2 in the Control register, read it out, check, and reset again afterwards? We could leave this feature out, sure, but now that it's on the table I think it would be nice to implement it at once.

leandrolanzieri · 2018-10-15T14:10:39Z

Can't you just write ones to RS1 and RS2 in the Control register, read it out, check, and reset again afterwards? We could leave this feature out, sure, but now that it's on the table I think it would be nice to implement it at once.

Yes, I think this could be easily done, and having the feature would be nice.

drivers/ds323x/ds3234.c

PeterKietzmann · 2018-12-18T16:31:49Z

ping @gebart.

PeterKietzmann · 2019-02-11T08:51:28Z

@leandrolanzieri how about you take over this PR? You base your changes on @gebart's commit and I'll test/review then. @gebart do you agree?

jnohlgard · 2019-02-13T12:59:22Z

@PeterKietzmann sounds good! I will not be able to work any more on this PR

leandrolanzieri · 2019-02-18T10:13:49Z

@PeterKietzmann I rebased and applied the suggested changes.

PeterKietzmann

@leandrolanzieri I left a few comments, please address them. Furthermore, what is your opinion about the pseudo-module preparation in this PR?

Other than that I ran the test on a nucleo-f103rb and got the expected 1Hz output

PeterKietzmann · 2019-02-18T10:54:02Z

drivers/ds323x/ds3234.c

+#include "ds323x_regs.h"
+#include "fmt.h"
+
+#define ENABLE_DEBUG    (1)


Please disable by default

PeterKietzmann · 2019-02-18T11:12:55Z

drivers/ds323x/ds3234.c

+        for (int k = 0; k <= 0x19; ++k) {
+            uint8_t dbg_reg = 0;
+            ds3234_read_reg(dev, k, 1, &dbg_reg);
+            print_byte_hex(k);


This has a dependency to the fmt module.

Changed that

PeterKietzmann · 2019-02-18T12:11:36Z

drivers/ds323x/include/ds3234_params.h

+#define DS3234_PARAM_SPI                (SPI_DEV(0))
+#endif
+#ifndef DS3234_PARAM_CS
+#define DS3234_PARAM_CS                 (SPI_HWCS(0))


I think GPIO_PIN(0, 0) would be the more common way here.

leandrolanzieri · 2019-02-18T15:59:06Z

Furthermore, what is your opinion about the pseudo-module preparation in this PR?

@PeterKietzmann I think it might be confusing to see that the DS323X family is there but actually only one device is supported. I believe for now we can skip that and just keep this minimal driver as DS3234 until an implementation of another device of the family is implemented.

PeterKietzmann

@leandrolanzieri thanks for addressing. Code looks good and still works. Please squash all commits to a single one!

This driver currently only supports the SPI connected DS3234, and only for initializing 1 Hz square wave output on the SQW pin, nothing else.

maribu · 2019-03-13T12:14:20Z

drivers/include/ds3234.h

+ */
+
+/**
+ * @defgroup    drivers_ds3234  DS3234 Extremely Accurate SPI RTC


@gebart: I think the @ingroup is missing here. As a result the documentation is shown directly in modues in the doxygen doc. Care to make a follow up PR to address this?

jnohlgard added this to the Release 2018.10 milestone Aug 24, 2018

jnohlgard requested a review from aabadie August 25, 2018 12:33

PeterKietzmann reviewed Aug 28, 2018

View reviewed changes

jia200x modified the milestones: Release 2018.10, Release 2019.01 Oct 25, 2018

leandrolanzieri reviewed Nov 13, 2018

View reviewed changes

drivers/ds323x/ds3234.c Show resolved Hide resolved

aabadie modified the milestones: Release 2019.01, Release 2019.04 Jan 25, 2019

leandrolanzieri force-pushed the pr/ds3234-pps branch from 6c7f604 to 25ed4f4 Compare February 18, 2019 10:00

PeterKietzmann reviewed Feb 18, 2019

View reviewed changes

leandrolanzieri removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 18, 2019

PeterKietzmann approved these changes Feb 19, 2019

View reviewed changes

drivers/ds3234: Minimal driver for DS3234 extremely accurate RTC

a6f430b

This driver currently only supports the SPI connected DS3234, and only for initializing 1 Hz square wave output on the SQW pin, nothing else.

leandrolanzieri force-pushed the pr/ds3234-pps branch from 860d924 to a6f430b Compare February 19, 2019 10:58

leandrolanzieri added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 19, 2019

leandrolanzieri merged commit 5d481c2 into RIOT-OS:master Feb 19, 2019

maribu reviewed Mar 13, 2019

View reviewed changes

leandrolanzieri mentioned this pull request Nov 5, 2019

core/kernel_defines: Introduce 'IS_ACTIVE' macro #12626

Merged

ds323x: Minimal driver for DS323x extremely accurate RTC #9838

ds323x: Minimal driver for DS323x extremely accurate RTC #9838

Uh oh!

Conversation

jnohlgard commented Aug 24, 2018

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

PeterKietzmann 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!

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!

jnohlgard commented Aug 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PeterKietzmann commented Aug 28, 2018

Uh oh!

jnohlgard commented Aug 28, 2018

Uh oh!

leandrolanzieri commented Sep 11, 2018

Uh oh!

jnohlgard commented Sep 11, 2018

Uh oh!

leandrolanzieri commented Sep 11, 2018

Uh oh!

leandrolanzieri commented Oct 11, 2018

Uh oh!

PeterKietzmann commented Oct 15, 2018

Uh oh!

leandrolanzieri commented Oct 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

PeterKietzmann commented Dec 18, 2018

Uh oh!

PeterKietzmann commented Feb 11, 2019

Uh oh!

jnohlgard commented Feb 13, 2019

Uh oh!

leandrolanzieri commented Feb 18, 2019

Uh oh!

PeterKietzmann 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!

Choose a reason for hiding this comment

Uh oh!

leandrolanzieri commented Feb 18, 2019

Uh oh!

jnohlgard commented Aug 28, 2018 •

edited

Loading

leandrolanzieri commented Oct 15, 2018 •

edited

Loading

maribu Mar 13, 2019 •

edited

Loading