flipperzero-firmware icon indicating copy to clipboard operation
flipperzero-firmware copied to clipboard

one_wire libs should not mandate calling furi_hal_ibutton_*

Open wdoekes opened this issue 2 years ago • 2 comments

Describe the enhancement you're suggesting.

TL;DR: I want to reuse one_wire, but switch pins from iBTN_Pin to a GPIO pin. Design decision needed.

I've hacked together a simple plugin that reads the temperature from a DS18B20 sensor.

I could reuse the lib/one_wire libs, except I had to change the pin:

-const GpioPin ibutton_gpio = {.port = iBTN_GPIO_Port, .pin = iBTN_Pin};
+const GpioPin ibutton_gpio = {.port = iBTN_GPIO_Port, .pin = LL_GPIO_PIN_3}; // <-- "B3" pin on top

This obviously breaks the ibutton functionality.

So, I'd like to reuse the one_wire lib, but be able to pass a different pin to it. For my purposes all uses of furi_hal_ibutton_* in ./lib/one_wire/one_wire_host.c (and ./lib/one_wire/one_wire_slave.c) should get an option to change pins.

An option could be to add function pointers to struct OneWireHost:

--- a/lib/one_wire/one_wire_host.c
+++ b/lib/one_wire/one_wire_host.c
@@ -4,6 +4,10 @@
 #include "one_wire_host_timing.h"
 
 struct OneWireHost {
+    // function pointers
+    void (*pin_low)(); // initialized to furi_hal_ibutton_pin_low
+    void (*pin_high)(); // initialized to furi_hal_ibutton_pin_high
+    // ... etc ...
     // global search state
     unsigned char saved_rom[8];
     uint8_t last_discrepancy;

But that feels like a lot of work when we just want the same behaviour but on a different pin.

An alternative option could be to rename all furi_hal_ibutton_* to furi_hal_onewire_* and use a global to switch pins. Something akin to this:

--- a/firmware/targets/f7/furi_hal/furi_hal_ibutton.c
+++ b/firmware/targets/f7/furi_hal/furi_hal_ibutton.c
@@ -122,12 +122,14 @@ void furi_hal_ibutton_remove_interrupt() {
     furi_hal_gpio_remove_int_callback(&ibutton_gpio);
 }
 
-void furi_hal_ibutton_pin_low() {
-    furi_hal_gpio_write(&ibutton_gpio, false);
+GpioPin* onewire_current_gpio = &ibutton_gpio;
+
+void furi_hal_onewire_pin_low() {
+    furi_hal_gpio_write(onewire_current_gpio, false);
 }
 
-void furi_hal_ibutton_pin_high() {
-    furi_hal_gpio_write(&ibutton_gpio, true);
+void furi_hal_onewire_pin_high() {
+    furi_hal_gpio_write(onewire_current_gpio, true);
 }
 
 bool furi_hal_ibutton_pin_get_level() {

But then we (a) have a lot of refactoring to do and (b) introduced a global GpioPin* onewire_current_gpio.

Do you have a suggestion which route to take? Or a third route?

Thanks for your answers! Walter

Anything else?

No response

wdoekes avatar Aug 13 '22 08:08 wdoekes

Agree, this thing should be customizable. Let's hear what @DrZlo13 thinks.

skotopes avatar Aug 13 '22 18:08 skotopes

@DrZlo13: I added a PR. It seems to work. But I'm not able to test the original (iButton) functionality. (And it may not fit your design.)

wdoekes avatar Aug 22 '22 10:08 wdoekes

Please check latest version.

skotopes avatar Mar 13 '23 09:03 skotopes

Looks like 7a3a1aaf0db35501b6ee5891ad039605285f2fea fixed it indeed :ok_hand:

wdoekes avatar Mar 13 '23 09:03 wdoekes