ds1307: initial import of a driver for the DS1307 RTC #7312

miri64 · 2017-07-04T11:34:11Z

While searching my Arduino Leonardo to test #7306 I found this breakout board, and while the board itself is discontinued (there is an updated version) the chip itself is still a popular RTC in the arduino community, as far as I know. So I decided to build some skills and develop my first own driver :-).

Depends on ~~#7311~~ (merged).

aabadie · 2017-07-04T11:41:05Z

I have one of these somewhere so I should be able to test.

miri64 · 2017-07-04T11:43:28Z

(I tested it on samr21-xpro FYI).

miri64 · 2017-07-04T11:44:01Z

Not sure if I should provide a separate test for the square wave generator.

aabadie

Went briefly into this one, just codewise. See comments inline.

You could also add some auto_init support, since you wrote added default params.

aabadie · 2017-07-04T11:44:12Z

drivers/ds1307/ds1307.c

+    return (res < 0) ? res : (int)mode;
+}
+
+static int _nvram_read(struct nvram *nvram, uint8_t *dst, uint32_t src,


nvram could be made const

This PR is not an API change of the nvram.h API.

I see, so that's fine then

aabadie · 2017-07-04T11:44:23Z

drivers/ds1307/ds1307.c

+static int _nvram_read(struct nvram *nvram, uint8_t *dst, uint32_t src,
+                       size_t size)
+{
+    ds1307_t *dev = nvram->extra;


const ds1307_t ?

aabadie · 2017-07-04T11:45:20Z

drivers/ds1307/ds1307.c

+    regs[DS1307_REG_SEC] = (bcd_from_byte(time->tm_sec) & DS1307_REG_SEC_MASK);
+    regs[DS1307_REG_MIN] = (bcd_from_byte(time->tm_min) & DS1307_REG_MIN_MASK);
+    regs[DS1307_REG_HOUR] = (bcd_from_byte(time->tm_hour) &
+                                DS1307_REG_HOUR_24H_MASK);


style: could be better aligned

aabadie · 2017-07-04T11:45:26Z

drivers/ds1307/ds1307.c

+    regs[DS1307_REG_HOUR] = (bcd_from_byte(time->tm_hour) &
+                                DS1307_REG_HOUR_24H_MASK);
+    regs[DS1307_REG_DOW] = (bcd_from_byte(time->tm_wday + DS1307_DOW_OFFSET) &
+                                DS1307_REG_DOW_MASK);


aabadie · 2017-07-04T11:45:32Z

drivers/ds1307/ds1307.c

+                                DS1307_REG_DOW_MASK);
+    regs[DS1307_REG_DOM] = (bcd_from_byte(time->tm_mday) & DS1307_REG_DOM_MASK);
+    regs[DS1307_REG_MON] = (bcd_from_byte(time->tm_mon + DS1307_MON_OFFSET) &
+                                DS1307_REG_MON_MASK);


aabadie · 2017-07-04T11:53:24Z

tests/driver_ds1307/main.c

+static ds1307_t dev;
+
+static struct tm init = {  /* Wed Sep 22 15:10:42 2010 is the author date of
+                         * RIOT's initial commit ;-) */


Maybe align comments

aabadie · 2017-07-04T11:53:47Z

tests/driver_ds1307/main.c

+
+static void test_get_time(void)
+{
+    for (int i = 0; i < 5; i++) {


Isn't used in the loop, so why?

aabadie · 2017-07-04T11:53:56Z

tests/driver_ds1307/main.c

+static void test_halt(void)
+{
+    ds1307_halt(&dev);
+    for (int i = 0; i < 3; i++) {


Isn't used in the loop, so why?

aabadie · 2017-07-04T11:54:07Z

tests/driver_ds1307/main.c

+
+int main(void)
+{
+    int res;


yes it is :-)

see ll. 141-142.

aabadie · 2017-07-04T11:56:27Z

drivers/include/ds1307.h

+ * @brief   Device descriptor for DS1307 devices
+ */
+typedef struct {
+    i2c_t i2c;                  /**< I2C bus the device is connected to */


These 2 attributes could be replaced by one ds1307_params_t

But ds1307_t also contains clk, which I don't need later on.

Ok, I see. Since it's only used during init, maybe using a macro would be enough. Then your device config just become:

typedef struct { ds1307_params_t params; nvram_t nvram; } ds1307_t; typedef struct { i2c_t i2c; uint8_t addr; } ds1307_params_t;

Then a lot of other param structs for I2C are false too. I don't think that would be correct.

I know, something that we can surely look into

Yes, but not in this PR. I think #6576 does this differently anyway.

miri64 · 2017-07-04T12:07:06Z

Addressed first batch of comments

aabadie · 2017-07-04T12:17:01Z

drivers/ds1307/ds1307.c

+    dev->addr = params->addr;
+
+    i2c_acquire(dev->i2c);
+    i2c_init_master(dev->i2c, params->clk);


You could also check if I2C can be initialized

miri64 · 2017-07-04T12:18:29Z

Missed some :-/.

aabadie

A few more comments

aabadie · 2017-07-04T12:17:46Z

drivers/ds1307/ds1307.c

+    return (res < 0) ? res : (int)mode;
+}
+
+static int _nvram_read(struct nvram *nvram, uint8_t *dst, uint32_t src,


I see, so that's fine then

aabadie · 2017-07-04T12:20:45Z

drivers/ds1307/include/ds1307_params.h

+#ifndef DS1307_PARAM_I2C_CLK
+#define DS1307_PARAM_I2C_CLK    (DS1307_I2C_MAX_CLK)
+
+#define DS1307_PARAMS_DEFAULT   {   .i2c = DS1307_PARAM_I2C, \


I don't get what you mean

aabadie · 2017-07-04T12:21:04Z

drivers/include/ds1307.h

+/**
+ * @brief   Maximum size in byte of on-chip NVRAM
+ */
+#define DS1307_NVRAM_MAX_SIZE   (56)


aabadie · 2017-07-04T12:24:48Z

drivers/include/ds1307.h

+ * @brief   Device descriptor for DS1307 devices
+ */
+typedef struct {
+    i2c_t i2c;                  /**< I2C bus the device is connected to */


Ok, I see. Since it's only used during init, maybe using a macro would be enough. Then your device config just become:

typedef struct { ds1307_params_t params; nvram_t nvram; } ds1307_t; typedef struct { i2c_t i2c; uint8_t addr; } ds1307_params_t;

aabadie · 2017-07-04T12:26:00Z

drivers/include/ds1307.h

+ * @param[in] params    device configuration (i2c bus, address and bus clock)
+ *
+ * @return      0 on success
+ * @return      -1 if unable to speak to the device


I insist on this one...

miri64 · 2017-07-04T12:40:16Z

drivers/include/ds1307.h

+    DS1307_SQW_MODE_1000HZ = 0x00,  /**< SQW: 1kHz */
+    DS1307_SQW_MODE_4096HZ = 0x00,  /**< SQW: 4.096 kHz */
+    DS1307_SQW_MODE_8192HZ = 0x00,  /**< SQW: 8.192 kHz */
+    DS1307_SQW_MODE_32768HZ = 0x00, /**< SQW: 32.768 kHz */


Oops, these are not supposed to be all 0. Will fix

miri64 · 2017-07-06T18:53:04Z

Rebased to current master. No longer waiting on other PR.

haukepetersen

Only one minor thing about the bus address, the rest looks good to me

haukepetersen · 2017-07-31T11:39:44Z

drivers/ds1307/include/ds1307_params.h

+#ifndef DS1307_PARAM_I2C_CLK
+#define DS1307_PARAM_I2C_CLK    (DS1307_I2C_MAX_CLK)
+
+#define DS1307_PARAMS_DEFAULT   {   .i2c = DS1307_PARAM_I2C, \


I would have done it without the tab alignment here, too, but IMHO there is no right and wrong, so this should be fine I guess.

haukepetersen · 2017-07-31T11:44:09Z

drivers/ds1307/include/ds1307_params.h

+#define DS1307_PARAM_I2C        (I2C_DEV(0))
+#endif
+#ifndef DS1307_PARAM_ADDR
+#define DS1307_PARAM_ADDR       (DS1307_I2C_ADDRESS)


The slave address is fixed to 0x68, so no need to parameterize it -> use DS1307_I2C_ADDRESS directly in the code.

haukepetersen · 2017-07-31T11:45:24Z

tests/driver_ds1307/main.c

+
+int main(void)
+{
+    int res;


yes it is :-)

haukepetersen · 2017-07-31T12:03:02Z

Tested on nucleo-l152 -> works as expected.

miri64 · 2017-07-31T12:39:26Z

Squashed.

miri64 · 2017-07-31T13:28:11Z

@aabadie you give your ACK as well?

miri64 · 2017-08-08T13:20:02Z

@aabadie ping?

aabadie

ACK, sorry for the late reply.

aabadie · 2017-08-16T07:25:55Z

and go

miri64 added Area: drivers Area: Device drivers Type: new feature The issue requests / The PR implemements a new feature for RIOT Area: timers Area: timer subsystems State: waiting for other PR State: The PR requires another PR to be merged first labels Jul 4, 2017

miri64 assigned haukepetersen Jul 4, 2017

miri64 requested review from haukepetersen and aabadie July 4, 2017 11:34

aabadie added this to the Release 2017.10 milestone Jul 4, 2017

aabadie requested changes Jul 4, 2017

View reviewed changes

aabadie reviewed Jul 4, 2017

View reviewed changes

aabadie requested changes Jul 4, 2017

View reviewed changes

miri64 commented Jul 4, 2017

View reviewed changes

aabadie mentioned this pull request Jul 5, 2017

drivers/si70xx: cleanup and use pseudomodules #7296

Merged

miri64 force-pushed the ds1307/feat/initial branch from 56504d6 to 2d9b665 Compare July 6, 2017 18:52

miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed State: waiting for other PR State: The PR requires another PR to be merged first labels Jul 6, 2017

haukepetersen requested changes Jul 31, 2017

View reviewed changes

miri64 force-pushed the ds1307/feat/initial branch from 1649867 to 162e0ae Compare July 31, 2017 12:35

miri64 added 2 commits July 31, 2017 14:39

ds1307: initial import of a driver for the DS1307 RTC

d07bfb4

tests: provide a test application for ds1307

94763ab

miri64 force-pushed the ds1307/feat/initial branch from 162e0ae to 94763ab Compare July 31, 2017 12:39

haukepetersen approved these changes Jul 31, 2017

View reviewed changes

aabadie approved these changes Aug 16, 2017

View reviewed changes

aabadie merged commit 5cb0be3 into RIOT-OS:master Aug 16, 2017

miri64 deleted the ds1307/feat/initial branch August 16, 2017 09:30

ds1307: initial import of a driver for the DS1307 RTC #7312

ds1307: initial import of a driver for the DS1307 RTC #7312

Uh oh!

Conversation

miri64 commented Jul 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aabadie commented Jul 4, 2017

Uh oh!

miri64 commented Jul 4, 2017

Uh oh!

miri64 commented Jul 4, 2017

Uh oh!

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

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!

miri64 commented Jul 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miri64 commented Jul 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

miri64 commented Jul 4, 2017 •

edited

Loading

miri64 commented Jul 4, 2017 •

edited

Loading