drivers: add generic 1-Wire bus driver #14897

haukepetersen · 2020-08-31T09:24:02Z

Contribution description

This PR adds a generic driver for interfacing with devices that use the Dallas Integrated 1-Wire bus (e.g. the ds18b20 temperature sensors). It allows to read/write specific devices, as well as scanning the bus for connected devices and to discover their ROM codes (addresses).

I had this code laying around for some years now and finally found some time to clean it up...

A PR for a ds18b20 device driver based on this PR will be opened separately.

Testing procedure

Connect one or more 1-Wire devices (e.g. ds18b20 sensors) to a board of your choice, flash the included test application (tests/driver_onewire), and run the init and discover shell commands.

Issues/PRs references

none

haukepetersen · 2020-08-31T09:37:34Z

pushed some minor doxygen and test app type fixes

benpicco

Nice addition, I have some comments.

drivers/onewire/onewire.c

drivers/onewire/onewire_str.c

drivers/include/onewire.h

benpicco · 2020-08-31T10:49:13Z

drivers/onewire/onewire_crc.c

+    0xC2, 0x9C, 0x7E, 0x20, 0xA3, 0xFD, 0x1F, 0x41,
+    0x00, 0x9D, 0x23, 0xBE, 0x46, 0xDB, 0x65, 0xF8,
+    0x8C, 0x11, 0xAF, 0x32, 0xCA, 0x57, 0xE9, 0x74
+};


I guess sys/checksum/crc8.c was too slow?

yes. Or at least I'd like to use this optimized version better then using the slower, generic solution...

IMO the implementation would still be better placed in sys/checksum, so that other potential users could also use the implementation.

One could even leave the decision on using the fast or the small CRC8 implementation to the user like this:

static inline uint8_t crc8(const uint8_t *data, size_t len, uint8_t g_polynom, uint8_t seed) { if (IS_USED(MODULE_CRC8_FAST) && __builtin_constant_p(g_polynom)) { switch (g_polynom) { case CRC8_POLY_DALLAS: return crc8_dallas(data, len, seed); } } return crc8_generic(data, len, g_polynom, seed); }

Here the crc8 function would pick the fast implementation over the generic if the module crc8_fast is used and the value of g_polynom is known at compile time (which is in all but the most obscure scenarios the case), so that the static inline function is always replaced with either a call to crc8_generic() or to the optimized crc implementation for the given polynomial (such as crc8_dallas()).

haukepetersen · 2020-08-31T12:08:23Z

addressed comments.

drivers/onewire/onewire.c

benpicco

I tried the test on avr-rss2 which has a DS18B20 connected to PD7.
Unfortunately the ATmega GPIO driver does not support GPIO_OD_PU, but according to the documentation there is a pull-up on the board.

So I tried GPIO_IN:

2020-08-31 15:29:53,752 #  init 3 7 2
2020-08-31 15:29:53,755 # Initializing 1-Wire bus at GPIO_PIN(3, 7)
2020-08-31 15:29:53,760 # 1-Wire bus successfully initialized
2020-08-31 15:32:50,303 # Discovery finished, found 10 devices:
2020-08-31 15:32:50,304 # Dev # - ROM code
2020-08-31 15:32:50,306 # [ 0]    0000000000000000
2020-08-31 15:32:50,309 # [ 1]    0000000000000080
2020-08-31 15:32:50,312 # [ 2]    0000000000000040
2020-08-31 15:32:50,312 # [ 3]    00000000000000C0
2020-08-31 15:32:50,315 # [ 4]    0000000000000020
2020-08-31 15:32:50,318 # [ 5]    00000000000000A0
2020-08-31 15:32:50,320 # [ 6]    0000000000000060
2020-08-31 15:32:50,321 # [ 7]    00000000000000E0
2020-08-31 15:32:50,324 # [ 8]    0000000000000010
2020-08-31 15:32:50,327 # [ 9]    0000000000000090

haukepetersen · 2020-08-31T14:26:15Z

I tried the test on avr-rss2 which has a DS18B20 connected to PD7

Hm, just remembered (from drivers/include/ds18.h):

@note Due to timing issues present on some boards this drivers features two

ways of reading information from the sensor. The optimized uses accurate

delays to handle this, while the second way polls the line for changes. If

you know that your board can handle ~3us resolution with the xtimer module,

then the optimized way is recommended. To used the optimized way add the

ds18_optimized module. Also this driver test application has a whitelist of

the boards this driver has been tested on and known to work.

Probably the above is the case for the 'avr-rss2' board?! That would explain the failed device scan. Seems like I have to do some magic as well for the onewire driver?! Damn, not getting this off my hands so easily as it seems...

benpicco · 2020-08-31T14:35:02Z

I tried tests/driver_ds18 and it would only print

2020-08-31 16:32:23,017 # Temperature [ºC]: -0.07

which I can assure you is not correct either.
Maybe @herjulf knows more? I'm using rev 2.3a of the board which has the sensor connected.

AFAIU xtimer is not suitable for anything that requires exact timings - we'd have to use a dedicated periph timer for that.
Fortunately most MCUs have plenty of those.

haukepetersen · 2020-08-31T14:42:19Z

Pretty sure this is because of bad timings on AVR-based hardware... Will add a polling mode as done in the already merged ds18 driver to see if that helps...

haukepetersen · 2020-08-31T14:50:17Z

If you try to build with USEMODULE=onewire_poll now, does that help?

benpicco · 2020-08-31T14:53:00Z

2020-08-31 16:52:21,823 #  init 3 7 2
2020-08-31 16:52:21,826 # Initializing 1-Wire bus at GPIO_PIN(3, 7)
2020-08-31 16:52:21,831 # 1-Wire bus successfully initialized
2020-08-31 16:52:25,344 #  discover
# stuck here forever

haukepetersen · 2020-08-31T15:05:36Z

Ok, so much for the quick and dirty fix... Will need to get home (where I have the fitting HW at hand) and see what I broke. Will post an update then.

herjulf · 2020-08-31T15:47:37Z

Hej,
Can only verify benpicco's testing. Even when using onewire_poll. Yes board uses external pull-up actually the 2.3a board has two one-wire buses.
init 3 7 2 (onboard DS18B20) and init 3 6 2 (external) with this test.

2020-08-31 16:52:21,823 #  init 3 7 2
2020-08-31 16:52:21,826 # Initializing 1-Wire bus at GPIO_PIN(3, 7)
2020-08-31 16:52:21,831 # 1-Wire bus successfully initialized
2020-08-31 16:52:25,344 #  discover
# stuck here forever

haukepetersen · 2020-08-31T15:49:49Z

Japp, as said, I screwed up with the quick-port of the polling mode. Need to investigate with HW at hand :-)

haukepetersen · 2020-09-14T11:48:02Z

but this seems not to be possible with this driver yet.

no, its not. Will add a comment in the doc addressing this.

kaspar030 · 2020-09-14T14:24:58Z

And I consider getting it to run on a high-level timer as personal challenge by now :-)

I'll try to help with that.

Can you post a baseline on how much overhead "xtimer_spin()" poses? and maybe also ztimer_spin() (which just landed some days ago). The latter takes at least 3us, but is fairly accurate after that, on nrf52840dk.

haukepetersen · 2020-09-14T14:29:58Z

will do once I have the hardware at my hands

herjulf · 2020-09-14T21:19:29Z

_pull(owi);
xtimer_usleep(1U);
_release(owi);
xtimer_usleep(1U);
_pull(owi);
xtimer_usleep(1U);
_release(owi);
xtimer_usleep(1U);

sleeping 1us gets 50us

kaspar030 · 2020-09-14T21:33:57Z

sleeping 1us gets 50us

Can you try xtimer_spin((xtimer_ticks32_t){1}) and ztimer_spin(ZTIMER_USEC, 1)? The latter probably needs USEMODULE += ztimer_usec somewhere in the makefile.

herjulf · 2020-09-14T22:03:33Z

Hmm
ztimer_spin ==> ztimer_sleep?

herjulf · 2020-09-14T22:26:29Z

@kaspar030

Better is seems. ztimer is confusing now...

kaspar030 · 2020-09-15T08:50:55Z

ztimer_spin ==> ztimer_sleep?

ztimer_spin() got merged fairly recently, maybe rebase to master?

herjulf · 2020-09-15T16:25:20Z

@kaspar030. Yes got it. Will test later...

herjulf · 2020-09-15T20:25:39Z

   ztimer_spin(ZTIMER_USEC, 1);
   _release(owi);
   ztimer_spin(ZTIMER_USEC, 1);
   _pull(owi);
   ztimer_spin(ZTIMER_USEC, 1);
   _release(owi);
   ztimer_spin(ZTIMER_USEC, 1);
   _pull(owi);
   ztimer_spin(ZTIMER_USEC, 1);
   _release(owi);

Something missing? First two spins is about 70us. Then..

kaspar030 · 2020-09-16T07:57:58Z

Something missing? First two spins is about 70us. Then..

Thanks @herjulf! Something seems quite off with ztimer on the arduino, I'll need to debug that.

herjulf · 2020-09-16T10:07:01Z

@kaspar030.
Yes onewire and small arduino are legacy and worked 10 years ago. onewire we still use for some external temp measurements. Otherwise BME280 (not 680) and SHT25.

Looking a ztimer... It should be possible to abstract even Mac Symbol Counter I hope?
This is a 32 bit timer/counter on the amegsl RF ones. It's atomic and has 3 HW threshholds via ISR. It is sourced via 16MHz xtal or the RTC 32kHz clock crystal. It has a fallback to RTC when you go to the lowest power modes and sync's when you switch power modes.
With this you can get very decent timing with few uA or less. IMO it can be worthwhile getting a RF atmega even if you don't use the radio. But it's
connected to the radio sort of drawback.

kaspar030 · 2020-09-16T10:16:46Z

Yes onewire and small arduino are legacy and worked 10 years ago.

is that using timers / bitbanging? or is it using a uart?

herjulf · 2020-09-21T13:11:13Z

Yes onewire and small arduino are legacy and worked 10 years ago.

is that using timers / bitbanging? or is it using a uart?

Simplest. bitbanging and busywaiting. DS1820 usually.
If I understand the RIOT arch the smallest timer is limited by the ISR plus the context switch time. Do we have any data/timings for context switch on different arch?
Also I need to understand sched_context_switch_request and context push and pop takes about 100 instructions for avr's. So is this the major overhead? Or is making the 8bit addresses atomic the problem? Or?

BTW I forgot to say the MAC Symbol Counter (MSC) is 16uS counts. I can get timer interrupts in RIOT now but it's not connected to anything useful. Also a question...

Still traveling for some days...

kfessel · 2020-09-23T13:51:25Z

onewire_t contains the line gpio_mode_t imode; /**< GPIO pin input mode, deducted from omode */
if it is deduced from omode there is no need to save it.
It is also very unlikely that this will change or first be known at runtime thus it should be configured (either into a define or a const) and compiled into, making the onewire_t a gpio pin.

stale · 2021-06-03T20:00:06Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

benpicco · 2021-09-29T11:07:59Z

This implementation suggests it's also possible to (ab)use UART for this.

herjulf · 2021-09-29T20:17:22Z

Thanks for pointing this possibility out. But the ultimate solution would be to keep the IRQ latency low. Remember seeing all the pushing and popping of registers for every IRQ. Well we do survive...

Enoch247 · 2022-05-04T18:55:41Z

I would like to take over this stale PR. Should I create a new PR, or is there a way to re-open it and assign it to me in such a way that I am edit the branch?

maribu · 2022-05-04T19:27:07Z

I can re-open it, but I have no way to assign it to you. So I guess the best is to open a new PR and post a link to that here.

herjulf · 2022-05-05T10:48:04Z

Yes please share any new links...

maribu · 2022-05-05T13:16:36Z

See #18051

Enoch247 · 2022-05-11T16:23:21Z

If I understand correctly, the items that I need to addressed to get this PR merged are:

the crc8 implementation needs moved into sys/checksum
make use of errno error codes
driver needs to be adapted to work with 8 bit AVRs; possibly by moving away from ztimer/xtimer
support needs added for parasitic powering of devices
remove not used param from onewire_params_t

Have I missed anything? I will use the PR I opened (#18051) to do this work.

haukepetersen added 2 commits August 31, 2020 11:12

drivers: add generic 1-Wire bus driver

4afd0bb

tests: add 1-Wire bus driver test application

5cab30c

haukepetersen added Type: new feature The issue requests / The PR implemements a new feature for RIOT Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Aug 31, 2020

haukepetersen requested review from aabadie, fjmolinas, leandrolanzieri, MichelRottleuthner and smlng as code owners August 31, 2020 09:24

haukepetersen mentioned this pull request Aug 31, 2020

drivers: add driver for DS18B20 temperature sensors #14899

Closed

haukepetersen added 2 commits August 31, 2020 11:35

fixup! driver: fix doxygen group

f49c776

fixup! test: cleanup Makefile and printf param type

2e7c65c

benpicco reviewed Aug 31, 2020

View reviewed changes

fixup! driver: aggregated fixes and enhancements

b9031c0

benpicco reviewed Aug 31, 2020

View reviewed changes

drivers/onewire/onewire.c Outdated Show resolved Hide resolved

benpicco reviewed Aug 31, 2020

View reviewed changes

drivers/onewire/onewire.c Outdated Show resolved Hide resolved

benpicco reviewed Aug 31, 2020

View reviewed changes

fixup! driver: remove casts and remove inline

cce4150

fixup! add onewire_poll submodule

e2fc384

fixup! finish inlining of onewire_print_rom

1a34e63

haukepetersen force-pushed the add_driver_onewire branch from 420cf10 to 1a34e63 Compare September 14, 2020 15:14

ArcticSnow mentioned this pull request Oct 2, 2020

RIOT Evaluation spectraphilic/wsn_riot#1

Closed

10 tasks

stale bot added the State: stale State: The issue / PR has no activity for >185 days label Jun 3, 2021

stale bot closed this Jul 8, 2021

kfessel mentioned this pull request May 4, 2022

DRAFT: Add onewire driver #18051

Closed

Enoch247 mentioned this pull request Jul 26, 2023

Draft: Add 1-Wire driver #19848

Draft

drivers: add generic 1-Wire bus driver #14897

drivers: add generic 1-Wire bus driver #14897

Uh oh!

Conversation

haukepetersen commented Aug 31, 2020

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

haukepetersen commented Aug 31, 2020

Uh oh!

benpicco left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

benpicco Aug 31, 2020

Choose a reason for hiding this comment

Uh oh!

haukepetersen Aug 31, 2020

Choose a reason for hiding this comment

Uh oh!

maribu Sep 13, 2020

Choose a reason for hiding this comment

Uh oh!

haukepetersen commented Aug 31, 2020

Uh oh!

Uh oh!

Uh oh!

benpicco left a comment

Choose a reason for hiding this comment

Uh oh!

haukepetersen commented Aug 31, 2020

Uh oh!

benpicco commented Aug 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

haukepetersen commented Aug 31, 2020

Uh oh!

haukepetersen commented Aug 31, 2020

Uh oh!

benpicco commented Aug 31, 2020

Uh oh!

haukepetersen commented Aug 31, 2020

Uh oh!

herjulf commented Aug 31, 2020

Uh oh!

haukepetersen commented Aug 31, 2020

Uh oh!

haukepetersen commented Sep 14, 2020

Uh oh!

kaspar030 commented Sep 14, 2020

Uh oh!

haukepetersen commented Sep 14, 2020

Uh oh!

herjulf commented Sep 14, 2020

Uh oh!

kaspar030 commented Sep 14, 2020

Uh oh!

herjulf commented Sep 14, 2020

Uh oh!

herjulf commented Sep 14, 2020

Uh oh!

kaspar030 commented Sep 15, 2020

Uh oh!

herjulf commented Sep 15, 2020

Uh oh!

herjulf commented Sep 15, 2020

Uh oh!

kaspar030 commented Sep 16, 2020

Uh oh!

herjulf commented Sep 16, 2020

Uh oh!

benpicco commented Aug 31, 2020 •

edited

Loading

kaspar030 commented Sep 16, 2020 •

edited

Loading