Skip to content

drivers/max31343: add driver for MAX31343 I2C RTC - #22115

Queued
Jakob-Mueller-97 wants to merge 3 commits into
RIOT-OS:masterfrom
Jakob-Mueller-97:MAX31343-Driver
Queued

drivers/max31343: add driver for MAX31343 I2C RTC#22115
Jakob-Mueller-97 wants to merge 3 commits into
RIOT-OS:masterfrom
Jakob-Mueller-97:MAX31343-Driver

Conversation

@Jakob-Mueller-97

Copy link
Copy Markdown

Contribution description

This PR adds a new device driver for the MAX31343 I2C real-time clock with
integrated MEMS oscillator.

The driver implements the periph_rtc interface, allowing it to serve as a
drop-in external RTC on any board that has an I2C bus. If another (internal) rtc already
provides the feature periph_rtc, the periph_rtc functions are omitted, but the device-specific API
can still be used directly, e.g. "max31343_get_time".

Supported features:

  • Time get/set via periph_rtc interface (rtc_get_time, rtc_set_time)
  • Alarm 1: set, get, enable/disable, clear
  • Oscillator power on/off
  • Square-wave output (1, 2, 4, 8, 16, 32 Hz)
  • Temperature sensor readout (centi-°C, i.e. 2350 = 23.50 °C)
  • Trickle charger configuration
  • Automatic temperature conversion mode and interval

The Oscillator Stop Flag (OSF) is checked during max31343_init(). If the
oscillator was stopped (e.g. after power loss), -ENODATA is returned to
signal that the stored time is invalid and must be set before use.

Note: interrupt-driven alarm callbacks via rtc_set_alarm() are not
supported. The INT pin must be handled externally via GPIO by the
application. cb and arg are accepted but intentionally ignored.

Datasheet:
MAX31343

Testing procedure

Tested on a nucleo-g070rb with the MAX31343 evaluation shield connected
via I2C.

Run the included driver test application:

make -C tests/driver_max31343 BOARD=nucleo-g070rb flash term

After flashing, run the built-in self-test from the shell:

> test
[test] setting reference time...
[test] reading back time...
[test] time readback OK
[test] waiting for clock to tick...
[test] time progression OK
[test] testing alarm set/get...
[test] alarm set/get OK
[test] testing alarm clear...
[test] alarm clear OK
[test] testing temperature read...
[test] temperature: 23.50°C  OK
[test] all tests PASSED

Issues/PRs references

None.

@github-actions github-actions Bot added Area: tests Area: tests and testing framework Area: build system Area: Build system Area: drivers Area: Device drivers labels Mar 2, 2026
@crasbe crasbe added Type: new feature The issue requests / The PR implemements a new feature for RIOT CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Mar 3, 2026
@riot-ci

riot-ci commented Mar 3, 2026

Copy link
Copy Markdown

Murdock results

✔️ PASSED

b500ea9 makefiles,sys/walltime: add support for MAX31343 RTC

Success Failures Total Runtime
11180 0 11180 12m:06s

Artifacts

@AnnsAnns

AnnsAnns commented Mar 4, 2026

Copy link
Copy Markdown
Member

Thank you for your contribution to RIOT, your commit appears to not be connected to your github account, try either adding your git config email into your github emails at https://github.com/settings/emails or changing it to one you have in there 😄

Other than that the PR already looks promising with very detailed comments for the driver. I have a feeling a certain @crasbe looked over this 🤨

@AnnsAnns AnnsAnns left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some minor nitpicks, I haven't tried this since I don't have the hardware for this, in general this looks really good already though

Comment thread drivers/include/max31343.h
Comment thread drivers/include/max31343.h Outdated
Comment thread drivers/max31343/include/max31343_internal.h
Comment thread drivers/max31343/include/max31343_internal.h Outdated
Comment thread drivers/max31343/include/max31343_params.h
Comment thread drivers/max31343/max31343.c
Comment thread drivers/max31343/max31343.c Outdated
Comment thread tests/drivers/max31343/main.c Outdated
Comment thread tests/drivers/max31343/main.c Outdated
Comment thread tests/drivers/max31343/main.c
@AnnsAnns

AnnsAnns commented Mar 4, 2026

Copy link
Copy Markdown
Member

Btw, while fine here since you had to to change the email, generally don't force push while your PR is getting reviewed, Githubs review flow starts really panicking sometimes and if somebody is actively reviewing this PR while it happens all review comments will be lost (I also did that once by accident 😔)

Comment thread tests/drivers/max31343/Makefile Outdated
@AnnsAnns

AnnsAnns commented Mar 4, 2026

Copy link
Copy Markdown
Member

Please look at the new static tests output, there are a lot of undefined things that need to have comments (even if I get why you didn't comment them since most of them feel obvious) and a few other formatting issues

@Jakob-Mueller-97

Copy link
Copy Markdown
Author

Thanks for the help. I tried my best to fix all of it.
I am not quite sure on how to fix the issue with the Makefiles. I am open to suggestions and happy to explain my thought process, if there are any questions.

@AnnsAnns

AnnsAnns commented Mar 9, 2026

Copy link
Copy Markdown
Member

I'd recommend looking at the static test output, I'd argue that most of them are fairly clear, e.g.:

A Makefile.dep is for dependency modelling only. Modify CFLAGS in a Makefile.include, in an app's Makefile, or (if only relevant to a single module/package) in the module's/package's Makefile:
  		drivers/max31343/Makefile.dep:7:      CFLAGS += -DUSE_INTERNAL_RTC

But feel free to explain your specific issue, maybe we can help.

@AnnsAnns AnnsAnns added the State: waiting for author State: Action by the author of the PR is required label Mar 9, 2026
@Jakob-Mueller-97

Copy link
Copy Markdown
Author

I'd recommend looking at the static test output, I'd argue that most of them are fairly clear, e.g.:

A Makefile.dep is for dependency modelling only. Modify CFLAGS in a Makefile.include, in an app's Makefile, or (if only relevant to a single module/package) in the module's/package's Makefile:
  		drivers/max31343/Makefile.dep:7:      CFLAGS += -DUSE_INTERNAL_RTC

But feel free to explain your specific issue, maybe we can help.

Thank you for the hint! I've moved the CFLAGS to Makefile.include and FEATURES_PROVIDED to Makefile.features as suggested by the static test.
However, I'm running into a problem with providing periph_rtc from this driver:
the goal is that applications can use the standard rtc_get_time() / rtc_set_time() API without knowing the underlying hardware. But from what I can tell, driver Makefile.features are not included during feature resolution — only board Makefile.features are. This means FEATURES_REQUIRED += periph_rtc in an app fails even though the driver provides it.

@benpicco benpicco left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some comments on the API.

I'm not sure how I feel about implementing the RTC API directly in the driver.
Ideally going forward applications should just use the walltime API instead of accessing the RTC directly. That way we can have time change notifications and easily implement different backends, see e.g. a282d31

Comment thread drivers/include/max31343.h Outdated
Comment thread drivers/include/max31343.h Outdated
Comment thread drivers/include/max31343.h Outdated
Comment thread drivers/include/max31343.h Outdated
Comment thread drivers/include/max31343.h Outdated
@Jakob-Mueller-97

Copy link
Copy Markdown
Author

I have some comments on the API.

I'm not sure how I feel about implementing the RTC API directly in the driver. Ideally going forward applications should just use the walltime API instead of accessing the RTC directly. That way we can have time change notifications and easily implement different backends, see e.g. a282d31

Thank you for your input.
Implementing the RTC API seems like more trouble that it is worth right now. It seems to just cause a bunch of Makefile problems.
I will take a look at the walltime API.

@crasbe crasbe left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall I'm very happy with the code, it looks very clean and structured. Below are some minor comments, mostly about styling related aspects.

I'll have to do a second round of review because I'm too tired now 😅

Comment thread drivers/include/max31343.h Outdated
Comment thread drivers/max31343/include/max31343_internal.h Outdated
Comment thread drivers/max31343/include/max31343_params.h Outdated
Comment thread tests/drivers/max31343/main.c Outdated
Comment thread drivers/max31343/max31343.c Outdated
Comment thread drivers/max31343/max31343.c Outdated
Comment thread drivers/max31343/max31343.c Outdated
Comment thread tests/drivers/max31343/main.c Outdated
Comment thread tests/drivers/max31343/main.c Outdated
Comment thread tests/drivers/max31343/main.c Outdated
@crasbe crasbe removed the State: waiting for author State: Action by the author of the PR is required label Mar 17, 2026
@github-actions github-actions Bot added the Area: sys Area: System label Mar 20, 2026
@github-actions github-actions Bot removed Area: SAUL Area: Sensor/Actuator Uber Layer Area: OTA Area: Over-the-air updates Area: CoAP Area: Constrained Application Protocol implementations Area: cpu Area: CPU/MCU ports Area: toolchain Area: toolchains; everything related to compilation, libc, linking, … Area: examples Area: Example Applications Area: Kconfig Area: Kconfig integration labels Mar 20, 2026
@Jakob-Mueller-97

Copy link
Copy Markdown
Author

That rebase went very wrong.

I am very sorry about that. I made a mistake somewhere and hope it is fixed now.

Comment thread drivers/include/max31343.h Outdated
Comment thread drivers/include/max31343.h Outdated
*/
typedef struct {
i2c_t i2c; /**< I2C bus the device is connected to */
max31343_sqw_freq_t sqw_freq; /**< SQW output frequency */

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to do this on init?
(and what is SQW?)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SQW is the squarewave output of the RTC, but arguably that can be set with the appropriate setter function.
Also the other parameters don't need to be in the parameters.

Fixed in d899775ac9

Comment thread drivers/include/max31343.h Outdated
*
* Writes the alarm time to the device. The alarm interrupt (A1IE) is
* disabled before writing and must be explicitly re-enabled afterwards
* using max31343_alarm_enable(), as required by the datasheet.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not enable the alarm when calling max31343_set_alarm() to mirror the RTC API?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that's because of the 1 second delay that's required. Not sure if I want to add ztimer as a dependency to wait for 1s in the setter function?

Comment thread drivers/include/max31343.h Outdated
* @retval 0 Success
* @retval -EIO I2C communication error
*/
int max31343_clear_alarm(const max31343_t *dev);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the difference between this and max31343_alarm_enable(dev, false)?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems redundant, I changed the name of max31343_alarm_enable to max31343_set_alarm_int and added the clearing of the Alarm Flag when enable == false.

f0704165d1

Comment thread drivers/include/max31343.h Outdated
i2c_t i2c; /**< I2C bus the device is connected to */
max31343_sqw_freq_t sqw_freq; /**< SQW output frequency */
bool trickle_enable; /**< enable trickle charger on init */
max31343_trickle_diode_t trickle_diode; /**< trickle charger diode path */

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
max31343_trickle_diode_t trickle_diode; /**< trickle charger diode path */
bool trickle_diode; /**< trickle charger diode path */

The Schottky diode is always enabled, so this only controls the trickle diode, right?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 5d73975b9a

Comment thread drivers/max31343/max31343.c Outdated
return _write_regs(dev, reg, &val, 1);
}

static int _validate_tm(const struct tm *t)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use rtc_tm_valid()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 7ff86c5d98

Comment thread drivers/max31343/max31343.c Outdated
Comment thread drivers/max31343/max31343.c Outdated
{
memset(t, 0, sizeof(*t));

int sec = bcd_to_byte(raw[0] & 0x7FU);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the RTC set those bits outside the value range?

Suggested change
int sec = bcd_to_byte(raw[0] & 0x7FU);
int sec = bcd_to_byte(raw[0]);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does, but I rather wonder why the _decode_time_regs and _encode_time_regs functions are only used once.

Incorporated in dd25c0cf16

Comment thread drivers/max31343/max31343.c Outdated
if (en) {
ie |= MAX31343_INTEN_A1IE;
} else {
ie &= (uint8_t)~MAX31343_INTEN_A1IE;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ie &= (uint8_t)~MAX31343_INTEN_A1IE;
ie &= ~MAX31343_INTEN_A1IE;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 6d1a9bc90d

Comment thread drivers/max31343/max31343.c Outdated
@AnnsAnns

Copy link
Copy Markdown
Member

That rebase went very wrong.

I am very sorry about that. I made a mistake somewhere and hope it is fixed now.

Don't worry too much about it, it happens to everyone at least once 😉

@crasbe

crasbe commented May 28, 2026

Copy link
Copy Markdown
Contributor

@Jakob-Mueller-97 Can you address the last review comments, so we can get this merged soon?

@crasbe crasbe added the State: waiting for author State: Action by the author of the PR is required label May 28, 2026
@crasbe crasbe self-assigned this May 28, 2026
@crasbe

crasbe commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Since Jakob Müller decided to not continue working on this despite having promised it after we gave him the final grade of his thesis, I'm taking over the PR now.

Disappointing to say the least.

@crasbe crasbe added AI: Co-Authored PR/Issue relies on AI. Documentation / Code was partly generated by AI. and removed State: waiting for author State: Action by the author of the PR is required labels Jul 27, 2026
@crasbe
crasbe force-pushed the MAX31343-Driver branch from a2c5556 to 02f28d3 Compare July 27, 2026 16:36
@github-actions github-actions Bot added the Area: doc Area: Documentation label Jul 27, 2026
@crasbe

crasbe commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

The walltime test fails because the MAX31343 RTC takes one second to update it's registers:

> time_set 2026-01-30T00:03:12
2026-07-27 19:17:12,775 # time_set 2026-01-30T00:03:12
2026-07-27 19:17:12,780 # time set to 2026-01-30T00:03:12
> time_get
2026-07-27 19:17:13,397 # time_get
2026-07-27 19:17:13,401 # current time: 2028-01-03T00:03:23
> time_get
2026-07-27 19:17:15,165 # time_get
2026-07-27 19:17:15,169 # current time: 2026-01-30T00:03:14
>

The self test does work though:

2026-07-27 19:20:56,748 # main(): This is RIOT! (Version: 2026.10-devel-149-g7b941-MAX31343-Driver)
2026-07-27 19:20:56,750 # MAX31343 RTC driver test
2026-07-27 19:20:56,750 #
2026-07-27 19:20:56,752 # MAX31343 initialized OK
> test
2026-07-27 19:20:58,740 # test
2026-07-27 19:20:58,742 # [test] setting reference time...
2026-07-27 19:21:00,746 # [test] reading back time...
2026-07-27 19:21:00,748 # [test] time readback OK
2026-07-27 19:21:00,752 # [test] waiting for clock to tick...
2026-07-27 19:21:02,755 # [test] time progression OK
2026-07-27 19:21:02,757 # [test] testing alarm set/get...
2026-07-27 19:21:02,762 # [test] alarm set/get OK
2026-07-27 19:21:02,765 # [test] testing alarm clear...
2026-07-27 19:21:02,768 # [test] alarm clear OK
2026-07-27 19:21:02,770 # [test] testing temperature read...
2026-07-27 19:21:02,773 # [test] temperature: 26.50°C  OK
2026-07-27 19:21:02,775 # [test] all tests PASSED

@Teufelchen1

Copy link
Copy Markdown
Contributor

I think you could squash before the next/last review comes in.

@crasbe
crasbe force-pushed the MAX31343-Driver branch 3 times, most recently from 4c46439 to 7a4ac9a Compare July 29, 2026 09:38
Jakob-Mueller-97 and others added 3 commits July 29, 2026 11:40
Co-authored-by: crasbe <crasbe@gmail.com>
Co-authored-by: crasbe <crasbe@gmail.com>
Co-authored-by: crasbe <crasbe@gmail.com>
@crasbe
crasbe force-pushed the MAX31343-Driver branch from 7a4ac9a to b500ea9 Compare July 29, 2026 09:41
@crasbe

crasbe commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

I think you could squash before the next/last review comes in.

Done 👍

@Teufelchen1 Teufelchen1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Soft ack, I read through it but didn't invest toooo much time ;) but given that crasbe cleaned it up and it's new code, the risk is minimal.

@crasbe
crasbe added this pull request to the merge queue Jul 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI: Co-Authored PR/Issue relies on AI. Documentation / Code was partly generated by AI. Area: build system Area: Build system Area: doc Area: Documentation Area: drivers Area: Device drivers Area: sys Area: System Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: new feature The issue requests / The PR implemements a new feature for RIOT

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants