Add 1-Wire driver - #19848
Conversation
github-actions
Bot
added
Area: tests
9b16584 to
cd75a9b
Compare
|
Nice PR ! |
Ok. I don't know much about RIOT's test system yet. I will work on getting up to speed and write some tests for this PR.
The The way I've been testing this API and driver so far is with a ds2433 and driver. The ds2433 driver is functional, but still needs work before I submit it for PR. |
crasbe
added
the
Type: new feature
d92b70d to
39a6876
Compare
39a6876 to
a8dd266
Compare
|
I think the test would need some more instructions, in a |
|
Yeah, the test is not really complete as is. After writing the test, I realized what a pain it is to define a bus and its params. So I'm reworking that now. I also went ahead and ported the ds18 driver to use the onewire API (coming in a separate PR). I've re-worked the API to use less RAM and ROM when only one back-end is enabled too. You should see some changes in this PR once I am finished. Pretty close... |
a8dd266 to
42e088c
Compare
github-actions
Bot
added
Area: doc
Enoch247
added
State: WIP
|
At this point. I don't expect to make any more big changes to the driver code. The test code still needs some work and doc. I'm also scratching my head on how the test could be made to be common as it really could be used to test any onewire backend. I hate the thought of each new backend just copying the test and have a bunch of near duplicates. |
@dylad : a few years later, yes. See my WIP branch for that. |
|
Unfortunately I don't have the Discovery at hand, but I tried with an nRF52840DK and a Nucleo-L152RE and L073RZ. The nRF52840DK just doesn't work, it does not recognize the DS18B20. Neither with the This is the log (with ENABLE_DEBUG = 1 in the The same happens on other GPIO pins as well and also with an external 4.7k Pull Up. It appears like some data is exchanged on the bus, but idk enough about OneWire to tell what might be the fault here. With Pullup: For good measure I tried with an 1k Pullup, but that leads to the same result. The sensor is a fake, but it does work. I checked it with an Arduino Uno and this tool: https://github.com/cpetrich/counterfeit_DS18B20 The Nucleos crash with a failed assertion: So it seems like it is the first assertion in the |
crasbe
added
the
CI: ready for build
Co-authored-by: crasbe <crasbe@gmail.com>
|
Grr. It would appear applying some of those suggests from withing the GitHub UI buggered up some of the comments. I will fix them locally... |
|
Whatever the outcome of PR #22457 becomes, I will give the same treatment to this PR. Either in this PR or the one that migrates the |
|
Hey @Enoch247, thank you for your contribution to RIOT! Please note that we require for all PRs to RIOT a declaration of AI-Tools / LLMs usage. It appears as if this section is missing in your PR. Please copy and fill in the section from .github/PULL_REQUEST_TEMPLATE.md. Thank you! |
github-actions
Bot
added
AI: Declaration Missing
|
I believe the failed header guard check ( |
It would seem that the problem is that the check is falsely identifying the |
There was a problem hiding this comment.
This is the culprit. The script checks for SOFT_ONEWIRE_PARAMS_H* because the classic headerguards could end with an underscore or just after the H.
I'll see if I can fix the headerguards check script.
There was a problem hiding this comment.
Ahh thanks! At any rate, I changed all the SOFT_ONEWIRE_PARAMS_* macros to drop the S at the end of PARAM so that they are in compliance with our driver writing tutorial and unified with the style of all other drivers.
|
I believe this PR is ready for a squash and merge, if there are not remaining concerns. |
The build still fails for some platforms 🤔 I don't know what's up with that, perhaps you have to ping another maintainer in Matrix with that. |
I'll look into it. |
|
I believe I fixed it. Waiting on CI to complete to find out. |
crasbe
left a comment
crasbe
left a comment
There was a problem hiding this comment.
I'll have to test it on Monday because I don't have Onewire hardware here.
There was a problem hiding this comment.
| #if IS_USED(MODULE_ONEWIRE_MULTIDRIVER) || DOXYGEN |
The IS_USED macro should be used to check if a module is used. Also for the following occurances.
There was a problem hiding this comment.
The coding convention would indicate that the current way I have done it is the preferred way.
If preprocessor conditionals are needed, use #ifdef MODULE_FOO or #if MODULE_FOO instead of #if IS_USED(MODULE_FOO)
There was a problem hiding this comment.
Perhaps you could add a sentence why you would choose either one.
There was a problem hiding this comment.
| Devices Inc) specified 1-Wire (or Onewire) Buses. 1-Wire slave device drivers should use it |
Perhaps add that so that's easier to search for.
|
First test with a After increasing the ISR stacksize arbitrarily to Attaching a DS18B20 temperature sensor does not change the behavior. With For reference: This is how I compile and flash the application: These are the changes I made to the cbuec@W11nMate:~/RIOTstuff/riot-vanillaice/RIOT$ git diff
diff --git a/tests/drivers/soft_onewire/Makefile b/tests/drivers/soft_onewire/Makefile
index 070eed4df7..52226300ff 100644
--- a/tests/drivers/soft_onewire/Makefile
+++ b/tests/drivers/soft_onewire/Makefile
@@ -5,6 +5,9 @@ include ../Makefile.drivers_common
#CFLAGS += -DSOFT_ONEWIRE_PARAMS_PIN="GPIO_PIN(PORT_C, 0)"
#CFLAGS += -DSOFT_ONEWIRE_PARAMS_TX_PIN="GPIO_PIN(PORT_C, 0)"
#CFLAGS += -DSOFT_ONEWIRE_PARAMS_RX_PIN="GPIO_PIN(PORT_C, 1)"
+CFLAGS += -DSOFT_ONEWIRE_PARAMS_PIN="GPIO_PIN(PORT_A,0)"
+
+CFLAGS += -DISR_STACKSIZE=8192
# uncomment to use a dedicated hardware timer for the 1-wire bus
# Note that TIMER_DEV(0) is often used by ztimer. This test does not need |
|
The behavior for the nRF52840DK is also unchanged: I do have a DS24BS33S+ EEPROM now which I'll test later. |
|
Same on the nRF52840DK with the EEPROM, using a 1k Pullup resistor. Also, when commenting out the timer lines in the Makefile, there is a failed assertion: |
|
Well that's a bummer. Digging through my other dev boards to see if I can reproduce these issues. |
|
Ok the failure on other boards was a silly mistake. I just missed a few renames of |
Co-authored-by: crasbe <crasbe@gmail.com>
4 participants
Contribution description
This patch adds a 1-Wire driver which can handle multiple driver implementations. Included in the patch is a soft 1-Wire driver implementation.
Testing procedure
Attach one or more 1-wire devices to pin C0 of an stm32f429i-disc1 eval board.
Apply the following patch:
Run
make -C tests/drivers/soft_onewire flashObserve the something like the following on the board's console:
Issues/PRs references
ds18driver (which is a 1-Wire device) more esp32 friendly.Declaration of AI-Tools / LLMs usage:
AI-Tools / LLMs that were used are: