Add hx711 ADC driver and saul by Citrullin · Pull Request #14039 · RIOT-OS/RIOT

Citrullin · 2020-05-07T10:34:28Z

Contribution description

HX711 ADC driver for weigh scales. Including SAUL implementation.

Testing procedure

`tests/driver_hx711`

Connect a HX711 board to a load cell on channel A.
Connect DT (DOUT) and SCK to a GPIO pin of your board.
Go into tests/drvier_hx711 and change the values of HX711_PARAM_SCK, HX711_PARAM_DOUT. If you use another board, change it as well.
Execute the test

SAUL

Connect the HX711 board to a weigh scale on channel A. Connect the hx711 breakout board to your microcontroller board.
Go into examples/saul and add the following to the Makefile. Change the values accordingly.

USEMODULE += hx711
CFLAGS += "-DHX711_PARAM_SCK=GPIO_PIN(1, 13)"
CFLAGS += "-DHX711_PARAM_DOUT=GPIO_PIN(1, 14)"

Flash the saul example and also use the target term.
Use saul write [ID] 200 to tare the scale.
Place a weight on your scale and execute saul read [TIMES] where [TIMES] is the number of reads. It should show the correct weight. If not, you have to adapt the divider. It is the value of HX711_PARAM_DIVIDER. If the weight result is too high, set a higher divider. If it is too low, you have to lower the divider. Until you have the correct value. You should use standardized weights in order to get precise results.

Issues/PRs references

Previous PR: #11416

jue89 · 2020-05-07T13:48:56Z

Hey @Citrullin!

Thanks for your contribution! One idea / question that came into my mind while reading over your code: Are there use cases that require one application to acquire different amounts of measurements for averaging?
If not, I would get rid of the times parameter and rely on on the configured dev.params.read_times. This would simplify the API.

It might be also good to split the driver into two separat commits: One for the driver itself and one for the saul wrapper.

Citrullin · 2020-05-07T14:01:58Z

Hey @Citrullin!

Thanks for your contribution! One idea / question that came into my mind while reading over your code: Are there use cases that require one application to acquire different amounts of measurements for averaging?
If not, I would get rid of the times parameter and rely on on the configured dev.params.read_times. This would simplify the API.

It might be also good to split the driver into two separat commits: One for the driver itself and one for the saul wrapper.

It changes the accuracy of the results. With 200 it is accurate, but also really slow. I think the test takes 2 minutes or something like this.

jue89 · 2020-05-08T11:33:37Z

It changes the accuracy of the results. With 200 it is accurate, but also really slow. I think the test takes 2 minutes or something like this.

So it might be useful to average more measurements during tare process than the actual measurement?!

Citrullin · 2020-05-08T15:48:00Z

It changes the accuracy of the results. With 200 it is accurate, but also really slow. I think the test takes 2 minutes or something like this.

So it might be useful to average more measurements during tare process than the actual measurement?!

Is an option, yes. Will change. One for normal ones and one for taring.

maribu

Some comments inline

maribu · 2020-07-02T19:00:44Z

drivers/hx711/Makefile

@@ -0,0 +1 @@
+include $(RIOTBASE)/Makefile.base


newline missing. (This is a configuration issue in your text editor. A correctly configured editor should never safe a text file without the newline. You might want to fix the configuration, so that you don't need to worry about this anymore.)

maribu · 2020-07-02T19:21:22Z

drivers/hx711/hx711.c

+    value.u8.data[2] = gpio_util_shiftin(dout, sck);
+    value.u8.data[1] = gpio_util_shiftin(dout, sck);
+    value.u8.data[0] = gpio_util_shiftin(dout, sck);


How are timing constraints of the hardware are enforced? The datasheet says a clock speed of at most 20 MHz is supported.

If this driver happens to be used on a faster board (e.g. lets say an ESP32 or an STM32F4xx or STM32F7xx) and the application is compiled with LTO enabled, a gpio_set(), a gpio_read() and a gpio_clear() are performed in a single CPU cycle each. Let's assume 8 CPU cycles are required per bit with LTO=1 on an ESP32 clocked at 240 MHz: gpio_util_shiftin() would then run with a clock of 30 MHz.

I think we can be pretty sure that any currently supported board with the current GPIO implementation would not get anywhere near the 20 MHz without LTO being used. But once all issues with LTO are resolved, it might get enabled by default.

Making the GPIO API inline-able is also something that could result in drastic speed ups of the GPIOs; and this is something that is not so unlikely to happen.

That's a really good point and would lead to an issue. They are not enforced. I need to think about this. Do you have an idea?

IMO, this issue could arise to all users of gpio_util_shiftin(), if LTO is enabled. So maybe the best is to extend it to also take a third argument specifying the maximum speed. This could be implemented using xtimer_periodic_wakeup()

If the GPIO ABC API proposed in #12015 would already be available, it would be even possible to provide a clock frequency argument (instead of a maximum clock frequency argument): With GPIO ABC it would be possible to match the clock frequency quite well. Maybe I should invest some time to polish that API and add "killer applications" on top of it to motivate the API.

So for now: The xtimer_periodic_wakeup() is the best option IMO. But I don't think that an API change will make it into the upcoming release, as this is not something usually merged during the feature freeze. But it is not too long until the next development cycle starts and PRs can of course be opened and reviewed at any point in time, only the merging is limited during feature freeze.

maribu · 2020-07-02T19:25:22Z

drivers/hx711/hx711.c

+    for (unsigned i = 0; i < gain; i++) {
+        gpio_set(sck);
+        gpio_clear(sck);
+    }


Here again the clock frequency could easily exceed the allowed maximum depending on the performance of the GPIO implementation

maribu · 2020-07-02T19:26:06Z

drivers/hx711/hx711.c

+    if (value.u8.data[2] & 0x80) {
+        value.u8.fill_byte = 0xFF;
+    }


sign extending 24 bits to 32 bits? If so, maybe add a comment

maribu · 2020-07-02T19:26:58Z

drivers/hx711/hx711.c

+
+int32_t hx711_get_units(hx711_t *dev, int8_t times)
+{
+    return (int32_t) (hx711_get_value(dev, times) / dev->params.divider);


Suggested change

return (int32_t) (hx711_get_value(dev, times) / dev->params.divider);

return hx711_get_value(dev, times) / dev->params.divider;

maribu · 2020-07-02T19:45:00Z

drivers/include/hx711.h

+#endif
+
+#endif /* HX711_H */
+/** @} */


missing newline

maribu · 2020-07-02T19:57:20Z

drivers/hx711/hx711.c

+int32_t hx711_read_average(hx711_t *dev, uint8_t times)
+{
+    int32_t sum = 0;
+    uint8_t i = times;
+
+    while (i--) {
+        sum += hx711_read(dev);
+    }
+
+    return sum / times;
+}


Is it needed to expose this function in the public API? If I don't miss anything, a user would call hx711_get_units() or hx711_tare() and never call this function directly, right? If so, please add static as qualifier

maribu · 2020-07-02T19:58:51Z

tests/driver_hx711/Makefile

+CFLAGS += "-DHX711_PARAM_SCK=GPIO_PIN(1, 13)"
+CFLAGS += "-DHX711_PARAM_DOUT=GPIO_PIN(1, 14)"
+
+include $(RIOTBASE)/Makefile.include


missing newline

maribu · 2020-07-02T19:59:40Z

tests/driver_hx711/Makefile

+include ../Makefile.tests_common
+
+BOARD ?= bluepill
+FEATURES_REQUIRED += periph_gpio


Suggested change

FEATURES_REQUIRED += periph_gpio

The driver depends on periph_gpio already. So no need to specify this twice

maribu · 2020-07-02T20:02:15Z

tests/driver_hx711/main.c

+
+    expect(value_before > value_after);
+    expect(value_after <= 1);
+


Maybe drop to the shell afterwards so that the SAUL integration can be tested using the saul command?

fjmolinas · 2020-09-29T10:33:15Z

ping @Citrullin :)

maribu · 2021-04-01T08:37:21Z

@tperchoc: Care to adopt this PR? (E.g. by opening a new PR based on this and keeping the copyright. Of course, extend the copyright and author list as needed.)

stale · 2022-03-02T09:19:02Z

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.

Citrullin · 2022-04-06T08:47:20Z

@tperchoc Should I just close this issue and you open another PR?

jwktje · 2022-04-15T15:21:35Z

@Citrullin I found this PR by looking for a way to use the HX711 in RIOT-OS. Do we need to wait for this PR to be merged or can I already use the HX711 somehow?

tperchoc · 2022-04-15T21:27:04Z

Hello, im alive just i didnt remember what i did here sry ;) just do has you want

Citrullin · 2022-04-18T10:19:32Z

@Citrullin I found this PR by looking for a way to use the HX711 in RIOT-OS. Do we need to wait for this PR to be merged or can I already use the HX711 somehow?

Would be great, if you just fork @tperchoc repo, pull the most recent main branch and create a new PR. I am happy to test it. :D

stale · 2022-11-02T02:28:33Z

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.

Citrullin · 2022-11-03T14:56:36Z

Good point. I am just talking to someone about it ^^

Citrullin requested review from MichelRottleuthner, aabadie, fjmolinas, leandrolanzieri and smlng as code owners May 7, 2020 10:34

Citrullin force-pushed the hx711 branch from 3335236 to 8af55ea Compare May 7, 2020 13:47

Citrullin added 2 commits May 7, 2020 17:09

Add hx711 driver

f76034a

Add SAUL wrapper for HX711

0316514

Citrullin force-pushed the hx711 branch from 8af55ea to 0316514 Compare May 7, 2020 15:10

miri64 added Area: drivers Area: Device drivers Area: SAUL Area: Sensor/Actuator Uber Layer Type: new feature The issue requests / The PR implemements a new feature for RIOT labels Jul 2, 2020

miri64 requested a review from maribu July 2, 2020 15:42

maribu reviewed Jul 2, 2020

View reviewed changes

This comment has been minimized.

Sign in to view

MrKevinWeiss added this to the Release 2021.07 milestone Jun 22, 2021

MrKevinWeiss removed this from the Release 2021.07 milestone Jul 15, 2021

stale bot added the State: stale State: The issue / PR has no activity for >185 days label Mar 2, 2022

stale bot removed the State: stale State: The issue / PR has no activity for >185 days label Apr 6, 2022

stale bot added the State: stale State: The issue / PR has no activity for >185 days label Nov 2, 2022

		@@ -0,0 +1 @@
		include $(RIOTBASE)/Makefile.base No newline at end of file

	return (int32_t) (hx711_get_value(dev, times) / dev->params.divider);
	return hx711_get_value(dev, times) / dev->params.divider;


		expect(value_before > value_after);
		expect(value_after <= 1);

Conversation

Citrullin commented May 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

tests/driver_hx711

SAUL

Issues/PRs references

Uh oh!

jue89 commented May 7, 2020

Uh oh!

Citrullin commented May 7, 2020

Uh oh!

jue89 commented May 8, 2020

Uh oh!

Citrullin commented May 8, 2020

Uh oh!

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

fjmolinas commented Sep 29, 2020

Uh oh!

This comment has been minimized.

maribu commented Apr 1, 2021

Uh oh!

stale bot commented Mar 2, 2022

Uh oh!

Citrullin commented Apr 6, 2022

Uh oh!

jwktje commented Apr 15, 2022

Uh oh!

tperchoc commented Apr 15, 2022

Uh oh!

Citrullin commented Apr 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stale bot commented Nov 2, 2022

Uh oh!

Citrullin commented Nov 3, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Citrullin commented May 7, 2020 •

edited

Loading

`tests/driver_hx711`

Citrullin commented Apr 18, 2022 •

edited

Loading