GPIOD Support through alternative implementation of the HWIF interface

[wrichter]

I'm running vzlogger on Fedora/RPi4 which no longer supports the legacy GPIO interface and has a changed MMAP layout. Neither of the existing approaches worked for me. Therefore I've implemented support for libGPIOD (which works both on Debian as well as Fedora).

In order to retain backwards compatibility with the config file syntax / not to introduce additional config fields, a GPIO line > 1000 will trigger the GPIOD implementation, and GPIO lines <1000 will trigger the old GPIO (/sys fs-based) implementation.

Currently there is no way to specify which gpiochip to use, it always defaults to gpiochip0.

[wrichter]

Am currently reworking this to prevent spurious readings - pls don't merge right now

[J-A-U]

I won't merge anything as long as the checks fail.

[wrichter]

sounds prudent :-) given that libgpiod is a new dependency, the test env needs to include it as well, otherwise it will continue to fail. How can this be done?

[r00t-]

@wrichter: add it here: https://github.com/volkszaehler/vzlogger/blob/master/.github/workflows/build%2Btest.yml#L8

[wrichter]

Thanks, am currently out and will do it when I'm back at home after next week.

[wrichter]

Hi @r00t- , hi @J-A-U can you please rerun the workflows to see if the code is now in better shape?

[wrichter]

the tests have additional #defines which interfere with the compilation… will work around

[wrichter]

@r00t- @J-A-U could you please run the workflows again?

[wrichter]

Thanks... so this is a bit annoying :-( the clang-format on my system creates a formatting incompatible with what the test requires:

wrichter@wrichter-mac vzlogger % uname -a
Darwin wrichter-mac 21.5.0 Darwin Kernel Version 21.5.0: Tue Apr 26 21:08:22 PDT 2022; root:xnu-8020.121.3~4/RELEASE_X86_64 x86_64
wrichter@wrichter-mac vzlogger % clang-format --version
clang-format version 14.0.4 
wrichter@wrichter-mac vzlogger % find include src tests  -type f -name '*.h' -o -name '*.cpp' -o -name '*.hpp' | xargs -I{} -P1 clang-format -i -style=file {}
wrichter@wrichter-mac vzlogger % git status
On branch gpiod
Your branch is up to date with 'origin/gpiod'.

nothing to commit, working tree clean
wrichter@wrichter-mac vzlogger %

Same when I try it from within a debian:stable container on another machine:

root@4d889486e0c9:/workdir# uname -a
Linux 4d889486e0c9 5.17.12-300.fc36.aarch64 #1 SMP Mon May 30 16:39:03 UTC 2022 aarch64 GNU/Linux
root@4d889486e0c9:/workdir# clang-format --version
Debian clang-format version 11.0.1-2
root@4d889486e0c9:/workdir# find include src tests  -type f -name '*.h' -o -name '*.cpp' -o -name '*.hpp' | xargs -I{} -P1 clang-format -i -style=file {}
root@4d889486e0c9:/workdir# git status
On branch gpiod
Your branch is up to date with 'origin/gpiod'.

nothing to commit, working tree clean
root@4d889486e0c9:/workdir#

Question 1: Do you have any tipps how to massage things so that the formatting is acceptable?

Question 2: The test fails because it doesn't link the GPIOD libs. This is my first exposure to CMake and I'm lost where to add them for the tests. Could you please help me? (for the vzlogger itself I made the following changes):

wrichter@wrichter-mac vzlogger % git diff master gpiod -- CMakeLists.txt src/CMakeLists.txt
diff --git a/CMakeLists.txt b/CMakeLists.txt
index c053b62..d278612 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -171,6 +171,8 @@ endif(WIN32)

 find_library(LIBUUID uuid)
 find_library(LIBGCRYPT gcrypt)
+find_library(GPIOD_LIBRARY NAMES libgpiod.so)
+

 include_directories(${CMAKE_BINARY_DIR})
 include_directories(${CMAKE_SOURCE_DIR}/include)
diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index cd76ec2..473b5d0 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -81,7 +81,7 @@ if( TARGET )
     target_link_libraries(vzlogger dl)
   endif( ${TARGET} STREQUAL "ar71xx")
 endif( TARGET )
-target_link_libraries(vzlogger ${CURL_STATIC_LIBRARIES} ${CURL_LIBRARIES} unistring ${GNUTLS_LIBRARIES} ${OPENSSL_LIBRARIES} )
+target_link_libraries(vzlogger ${CURL_STATIC_LIBRARIES} ${CURL_LIBRARIES} unistring ${GNUTLS_LIBRARIES} ${OPENSSL_LIBRARIES} ${GPIOD_LIBRARY})

 # add programs to the install target
 INSTALL(PROGRAMS
wrichter@wrichter-mac vzlogger %

[leoguiders]

Hi Wolfram,

found your gpiod branch while trying to get S0 working on a Raspberry Pi 3B. Thanks for your work, I'm currently testing the gpiod support with my S0-meter.

In order to retain backwards compatibility with the config file syntax / not to introduce additional config fields, a GPIO line > 1000 will trigger the GPIOD implementation, and GPIO lines <1000 will trigger the old GPIO (/sys fs-based) implementation.

This does feel hacky.

Currently there is no way to specify which gpiochip to use, it always defaults to gpiochip0.

The "device" configuration-string is currently unused when setting the gpio value. That would be a good place to set the gpiochip. Maybe not less hacky than the current solution, but what about switching to gpiod when gpio is set and device string starts with "/dev/gpiochip" (or gpiod_is_gpiochip_device(device) returns true for that matter) ?

[wrichter]

Hi Wolfram,

found your gpiod branch while trying to get S0 working on a Raspberry Pi 3B. Thanks for your work, I'm currently testing the gpiod support with my S0-meter.

In order to retain backwards compatibility with the config file syntax / not to introduce additional config fields, a GPIO line > 1000 will trigger the GPIOD implementation, and GPIO lines <1000 will trigger the old GPIO (/sys fs-based) implementation.

This does feel hacky.

It sure is. Of course we could also introduce a new Boolean config parameter use_gpiod which defaults to false. Cleaner would be to just create a new config parameter hardware_interface_impl that has values like „uart“, „gpio_sysfs“, „gpio_mmap“, „gpiod“ and „legacy“ and defaults to „legacy“. Then existing users can continue to use the magic decisiontree to decide whether to use the uart/gpio/mmap implementations and new users have a single place to specify which implementation they want. This feels like a separate pull request though.

Currently there is no way to specify which gpiochip to use, it always defaults to gpiochip0.

The "device" configuration-string is currently unused when setting the gpio value. That would be a good place to set the gpiochip. Maybe not less hacky than the current solution, but what about switching to gpiod when gpio is set and device string starts with "/dev/gpiochip" (or gpiod_is_gpiochip_device(device) returns true for that matter) ?

again laziness on my side. I like the idea - would you want to send a pull request to https://github.com/wrichter/vzlogger/tree/gpiod ?

[leoguiders]

I'm still trying to get my head around what is required and what is the most user-friendly way to configure this.

The question here really is - do we need do support /sysfs and gpiod at the same time or can we just assume that gpiod is the better way to deal with gpios and use it if available and fallback to /sysfs if it isn't?

[wrichter]

so I personally do think that GPIOD is the better solution, and I have been running the implementation successfully for the last ~4 months. But it hasn't seen much testing beyond my setup, is not yet merged with the main development branch, etc... I expect both versions will need to be supported in parallel until the GPIOD version has seen more testing. At the moment it can't even be merged since some tests fail - which I believe is currently caused by something else than my code, but also the tests don't yet know anything about GPIOD (which was the reason it failed earlier - see https://github.com/volkszaehler/vzlogger/pull/525#issuecomment-1170490110 ).

[trabant-asb]

May I ask why active_low ist set true in MeterS0.cpp when using libgpiod?

	if (_configureGPIO) {
		print(log_info, "configuring GPIO via GPIOD (active low)", "S0");
		flags = GPIOD_LINE_REQUEST_FLAG_ACTIVE_LOW;
	}

In the sysfs implementation it is set to 0:

 name.append("/sys/class/gpio/gpio");
                name.append(std::to_string(_gpiopin));
                name.append("/active_low");
                fd = ::open(name.c_str(), O_WRONLY);
                if (fd < 0)
                        throw vz::VZException("open active_low failed");
                res = ::write(fd, "0\n", 2);
                if (2 != res)
                        throw vz::VZException("set active_low failed");
                if (::close(fd) < 0)
                        throw vz::VZException("set active_low failed");

In my understanding vzlogger expects a pulled down gpio bin, which receives positive impulses from the S0 meter. setting active_low to true would kind of invert this logic.

[wrichter]

May I ask why active_low ist set true in MeterS0.cpp when using libgpiod?

I did not look at the sysfs implementation since that was a non-starter on Fedora. When designing my system I took a look at chapter 8 in https://www.elektronik-kompendium.de/sites/raspberry-pi/2006051.htm (German) which basically says "it probably doesn't matter much, but a pull-up resistor and signalling by connecting the pin to ground is a bit more interference-resistant".

[trabant-asb]

May I ask why active_low ist set true in MeterS0.cpp when using libgpiod?

I did not look at the sysfs implementation since that was a non-starter on Fedora. When designing my system I took a look at chapter 8 in https://www.elektronik-kompendium.de/sites/raspberry-pi/2006051.htm (German) which basically says "it probably doesn't matter much, but a pull-up resistor and signalling by connecting the pin to ground is a bit more interference-resistant".

While I agree that it won't matter much from the technical perspective, I see a consistency issue here. vzlogger was originally written to work with certain hardware extensions like https://wiki.volkszaehler.org/hardware/controllers/raspberry_pi_erweiterung_mit_schaltausgaengen_rev.1, which to my best knowledge use the pull-down logic.

And people (like me) who started with the VZ at a time when the extensions were not available any longer, learned that a pull-down hard- or soft-resistor is the easiest way to make it work, because it works that way with the default configure_gpio setting in vzlogger.conf, without the need to configure the gpio pin manually or edit the vzlogger code.

So, if any person with a working S0 config changes from sysfs to gpiochip, he or she could likely see issues. For example, vzlogger will not count impulses any more, but the time between impulses - which indeed will generate some output, but it might be erroneous.

So from that perspective I think both sysfs as well as the gpiochip implementation (which is definitely the way to go!) should do the same default pin setup.

In my personal opinion some new configuration parameters could however make sense, like for your use case, or indeed, for setting the soft resistors with GPIOD_LINE_REQUEST_FLAG_BIAS_PULL_DOWN or _UP, which is a new capability and could not be done with sysfs at all.

[wrichter]

Legacy makes a compelling case :-) My hardware lives on a breadboard, so I can somewhat simply rewire towards a pulldown resistor. I believe that this should be configurable and default to whatever is similar to the implementations out there.

Properly making stuff configurable was the discussion earlier in this thread; see https://github.com/volkszaehler/vzlogger/pull/525#issuecomment-1294839372

While we're pondering about the best way, you can get to an "active high" behavior by setting configureGPIO to false (it's the only thing that the GPIOD code configures, the other aspects like which level/edge to react to are haredcoded).

[trabant-asb]

Edited my last post - the part with the soft resistor flags did not make sense... Regarding the configuration options, it might make sense to keep configureGPIO in vzlogger.conf, with true doing the same as with the sysfs configuration, but allow detailed options like active_low, bias_pull_down and so on.

[trabant-asb]

I expect both versions will need to be supported in parallel until the GPIOD version has seen more testing.

So far I have tested the gpiod based vzlogger for a few days in my environment (S0 cyble sensor attached to my gas meter) and didn't see any problems. I do like the states mechanism in general and especially the high_wait parameter, which I could tune to the pulse length from the spec of the electronic (cyble) sensor and thereby avoid some rare electrostatic interference to be counted... setting debounce_delay to 0 and high_wait to 60 did the trick.

The question here really is - do we need do support /sysfs and gpiod at the same time or can we just assume that gpiod is the better way to deal with gpios and use it if available and fallback to /sysfs if it isn't?

Imho, this makes sense for the time being... with the current linux kernels, both options can be enabled at compile time, however as I understand it the gpiochip solution will finally replace the sysfs way.

One more related configuration thing that was referenced in this thread...

The "device" configuration-string is currently unused when setting the gpio value. That would be a good place to set the gpiochip.

... defaulting to /dev/gpiochip0 when the device option is not set?

I'm not sure if I can be of any help in bringing this PR forward to be merged, having no idea why the github build fails; however I could try to implement the idea I had with that configureGPIO options....

Regarding the configuration options, it might make sense to keep configureGPIO in vzlogger.conf, with true doing the same as with the sysfs configuration, but allow detailed options like active_low, bias_pull_down and so on.

Not sure if any of my thoughts are helpful... let me know what you think.

[wrichter]

@trabant-rgb Agree with your comments. I suppose I need to reach out to the developers mailing list to get help; the GitHub discussion does not create much interaction with the project maintainers. if I only had more free time…

[wrichter]

Thanks to @r00t- 's merge the unit tests are now completing successfully: https://github.com/wrichter/vzlogger/actions/runs/3943901824/jobs/6749257157

[r00t-]

wow, you almost almost beat me to fix the unit tests.
(we (also me) have merged @maxberger's fixes so master builds and tests successfully again, so does your change with master merged. (would prefer rebase, as i did in #565, there's one trivial conflict.)) would also suggest to take my change to add some verbosity to cmake.

[r00t-]

code-wise i'm unsure what to make of this. i haven't had time to review the code. we probably want support for this. unsure if this should be optional and/or runtime-configured.

[wrichter]

would also suggest to take my change to add some verbosity to cmake.

Thanks, did that.

code-wise i'm unsure what to make of this. i haven't had time to review the code. we probably want support for this. unsure if this should be optional and/or runtime-configured.

I'm happy to continue working on that, once the direction is clearer.

[trabant-asb]

I was using the gpiod branch very successfully with an electronic (Cyble) sensor using the high_wait option and no debounce_delay. Now I moved and have a different gas meter with a reed contact. Short question - as I understand it the high_wait will start counting after the debounce_delay?

So, if both debounce_delay and high_wait are set to 30ms, a reading will be recognized after a total of 60ms, or am I getting this wrong?

[wrichter]

I was using the gpiod branch very successfully with an electronic (Cyble) sensor using the high_wait option and no debounce_delay. Now I moved and have a different gas meter with a reed contact. Short question - as I understand it the high_wait will start counting after the debounce_delay?

So, if both debounce_delay and high_wait are set to 30ms, a reading will be recognized after a total of 60ms, or am I getting this wrong?

Correct. The assumption is that during debounce_delay, the signal might be jumping between 0 and 1 multiple times - so we're just waiting for it to become stable. Typical times for mechanical bouncing are between 100 µs to 10 ms. After debounce, if high_wait is configured, once a high is recognized it waits another period to ensure there's a minimum duration high signal before it is considered valid - for me the use case was to eliminate induced voltage spikes because my whole setup lives in an electromagnetically suboptimal environment.

[trabant-asb]

Ok, so I've set the debounce_delay to 10 and the high_wait to 50. Indeed all real impulses have been counted over 24 hours or so and a lot of noise is filtered. Sometimes the state machine plays back and forth from 2 to 3 without counting an impulse, which as I understand is exactly what it should do.

On two occasions, my vzlogger run into a situation, where the state went never to 0 between two readings, and indeed that second reading was not a real one...

First transcript - this is how it should look:

[Jun 12 14:19:52][S0]   [7604.426069902] GPIO event 1 (1=rising edge, 2=falling edge) in current state 0
[Jun 12 14:19:52][S0]   MeterS0:HWIF_GPIOD: state machine transition from 0 to 2 with a timeout at 1686572392.386226232
[Jun 12 14:19:52][S0]   MeterS0:HWIF_GPIOD: state machine transition from 2 to 3 with a timeout at 1686572392.436248305
[Jun 12 14:19:52][S0]   MeterS0:HWIF_GPIOD: state machine transition from 3 to 1 with a timeout at -1.-1
[Jun 12 14:19:52][chn0] Adding reading to queue (value=1.00 ts=1686572392445)

That 14:19 reading was a real one, but as you see it is not followed by a falling_edge. This happens when the gas meter runs really slow, in my case when only the pilot flame of my boiler is idling.

Then, see the following transcript what happens next...

[Jun 12 14:28:28][S0]   [8120.555918407] GPIO event 1 (1=rising edge, 2=falling edge) in current state 1
[Jun 12 14:28:28][S0]   [8120.556014343] GPIO event 1 (1=rising edge, 2=falling edge) in current state 1
[Jun 12 14:28:28][S0]   [8120.556022781] GPIO event 1 (1=rising edge, 2=falling edge) in current state 1
[Jun 12 14:28:28][S0]   [8120.556029291] GPIO event 2 (1=rising edge, 2=falling edge) in current state 1
[Jun 12 14:28:28][S0]   MeterS0:HWIF_GPIOD: state machine transition from 1 to 2 with a timeout at 1686572908.516320672
[Jun 12 14:28:28][S0]   [8120.556036739] GPIO event 1 (1=rising edge, 2=falling edge) in current state 2
[Jun 12 14:28:28][S0]   MeterS0:HWIF_GPIOD: state machine transition from 2 to 3 with a timeout at 1686572908.556407338
[Jun 12 14:28:28][S0]   [8120.556041218] GPIO event 1 (1=rising edge, 2=falling edge) in current state 3
[Jun 12 14:28:28][S0]   MeterS0:HWIF_GPIOD: state machine transition from 3 to 1 with a timeout at -1.-1
[Jun 12 14:28:28][S0]   [8120.556048302] GPIO event 1 (1=rising edge, 2=falling edge) in current state 1
[Jun 12 14:28:28][S0]   [8120.556053406] GPIO event 1 (1=rising edge, 2=falling edge) in current state 1
[Jun 12 14:28:28][S0]   [8120.556055333] GPIO event 1 (1=rising edge, 2=falling edge) in current state 1
[Jun 12 14:28:28][S0]   [8120.556062468] GPIO event 1 (1=rising edge, 2=falling edge) in current state 1
[Jun 12 14:28:28][S0]   [8120.556067728] GPIO event 1 (1=rising edge, 2=falling edge) in current state 1
[Jun 12 14:28:28][S0]   [8120.556074708] GPIO event 1 (1=rising edge, 2=falling edge) in current state 1
[Jun 12 14:28:28][chn0] Adding reading to queue (value=1.00 ts=1686572908516)

Some spurious rising and falling edges are seen, as I see neither of them came actually from the gas meters reed contact. The rising_edges are ignored because the state_machine is already in state 1/high. Then, a falling_edge comes and causes the state_machine to switch to 2/debounce. It never went to 0/low which seems correct to me.

But then, another rising_edge causes the state machine to go to 3/high_wait and further back to 1/high, following by a wrong reading being counted, even if the state_machine was never at 0/low between the readings. Maybe sort of a race condition?

[wrichter]

But then, another rising_edge causes the state machine to go to 3/high_wait and further back to 1/high, following by a wrong reading being counted, even if the state_machine was never at 0/low between the readings. Maybe sort of a race condition?

As of today, the GPIOD code will recognize a new signal whenever it changes its state towards HIGH, even if the sequence is HIGH->DEBOUNCE->HIGH. This means there is no minimal LOW time enforced.

The underlying question is: what is the correct behaviour if the signal is HIGH, a falling edge is detected and then a rising edge causes the signal to become HIGH again.

[trabant-asb]

The underlying question is: what is the correct behaviour if the signal is HIGH, a falling edge is detected and then a rising edge causes the signal to become HIGH again.

Hmm... as I see it the debounce_time applies to both rising_ and falling_edges. So in this case the falling_edge sends the state machine to 2/debounce with a timeout, to see if that signal is stable. While I am unsure what the specifications are, in my opinion (and in my use case) a contraindicating signal like a rising_edge during debounce_time of a falling_edge should set the state engine back to 1 but not count it as a reading.

[wrichter]

Ok this might be minor semantics, but what you seem to want is a LOW_WAIT state that enforces a minimum duration low signal between two highs. Debounce is really just about ignoring additional edges within a certain time frame after the first rising or falling edge has been detected, so imperfect switches that don’t create a clean signal aren’t counted multiple times.