GPIOD Support through alternative implementation of the HWIF interface #525
Conversation
|
Am currently reworking this to prevent spurious readings - pls don't merge right now |
|
I won't merge anything as long as the checks fail. |
|
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? |
|
Thanks, am currently out and will do it when I'm back at home after next week. |
|
the tests have additional #defines which interfere with the compilation… will work around |
|
Thanks... so this is a bit annoying :-( the clang-format on my system creates a formatting incompatible with what the test requires: Same when I try it from within a debian:stable container on another machine: 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): |
|
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.
This does feel hacky.
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) ? |
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.
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 ? |
|
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? |
|
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 #525 (comment) ). |
|
May I ask why active_low ist set true in MeterS0.cpp when using libgpiod? In the sysfs implementation it is set to 0: 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. |
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 |
|
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 #525 (comment) While we're pondering about the best way, you can get to an "active high" behavior by setting configureGPIO to |
|
Edited my last post - the part with the soft resistor flags did not make sense... |
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
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...
... 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
Not sure if any of my thoughts are helpful... let me know what you think. |
|
@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… |
|
Yeah I noodled over it a bit as well. I have coded up a quick solution which adds a counter that is incremented whenever the HIGH state is entered, and reset to 0 when the LOW state is entered. A pulse is only detected at a state transition to HIGH when the counter is exactly 1. This means only the first transition from LOW to HIGH will be considered, HIGH->DEBOUNCE->HIGH transitions will have a counter value greater than 1. I'm testing it tonight to see if this works. |
Any chance to get that new MeterS0.cpp to test it in my environment? |
|
I just pushed the commit - if you use the latest version in my gpiod branch, it includes the counter a5551bc |
That was fast... I've run the new code over the night and reviewed my log file this morning. Indeed on several occasions it has prevented wrong impulses to be counted. |
|
Just to give you some positive feedback... I did run your code with the electronic cyble sensor about half a year and am now using it with your latest code changes for about 2 months with a vendor supplied reed contact on another meter. I have a lot of electrostatical noise (which I see when turning on logging) and am running the sensor directly on a Raspberry GPIO line. In spite of this non-optimal environment, I am not getting any irregular readings logged. Good job! |
|
I tried using your branch but I cannot get vzlogger to start: Here is my config: "protocol" : "s0",
"enabled" : true,
"gpio" : 1024,
"configureGPIO" : false,
"debounce_delay" : 30,
"high_wait" : 250,
"channels": [{
"uuid" : "918f7ff0-ae33-11ee-9c97-b19e2a89dc6a",
"middleware" : "http://odroidxu4.local:8080",
"identifier" : "Impulse"What am I missing? |
|
Errno 16 is "device or resource is busy". Can you check with gpioinfo https://www.acmesystems.it/gpiod whether line 24 is indeed free to use? |
|
It seems to be used by sysfs: Do I need to specifically disable sysfs somehow? |
|
Rebooting the PI did not help, now the line is used by sysfs and set as output. |
|
|
|
good question, I don't know. I'm personally using a Fedora IoT image which doesn't provide SysFS. |
|
Now that I have the code running, looks like some tweaks to the config or the set-up are required. I can reliably trigger invalid events by turning on CCFL tube light with a magnetic ballast. This is a correct event: This happens if I turn on the lamp: 3 events were added to the queue as a result. The question is whether I am better off trying to tweak the settings or maybe switching to an active_low approach would be a better choice. According to the specification of the IN-Z62, the impulse should be at least 0,25 s. |
|
I tried 1000 ms high wait and the lamp is still able to generate an impulse. |
Yes! See point 14.c) IX.: https://wiki.volkszaehler.org/howto/building_raspberry_pi_image_for_vz |
|
So am I understanding correctly that the CCFL tube generates noise for ~4 seconds (13:02:29 to 13:02:33)?. Not sure what kind of logic could reliably distinguish between that and a legit signal. Is it possible to shield the CCFL or the line somehow? In my home, I only need to work around a spurious spike by an ELTAKO relay. |
Looks like the lamp is my final boss indeed. I could rewire it for LED removing the ballast, maybe it would reduce the noise. Other than that, I could also see if I can use a shielded Cat cable instead of an unshielded twisted wire [1]. But first I will see if using an active_low approach changes anything. [1] https://www.berrybase.de/kupferschaltdraht-isoliert-oe2x0-5mm-rot-weiss-10m |
|
Turns out there is indeed value in shorting to ground as opposed to 3V3. I have moved to GPIO7 to benefit from the default pull-up state and the spurious signals from the lamp went away (turned on at 15:17:30 and off at 15:17:41), even with the "protocol" : "s0",
"enabled" : true,
"gpio" : 1007,
"configureGPIO" : true,
"debounce_delay" : 30,
"high_wait" : 250,
"channels": [{
"uuid" : "918f7ff0-ae33-11ee-9c97-b19e2a89dc6a",
"middleware" : "http://odroidxu4.local:8080",
"identifier" : "Impulse"
}] |
|
With the new approach, the counter seems really stable: |
|
this discussion is full of noise, OR is there any indication that the stray impulses issues are related to the code? @belegdol: can you compare the performance to the old implementation? |
|
@r00t- fair enough, apologies.
|
|
To me it sound like the main improvement was due to the change in shielding (GPIO7 and grounding) and not due to the performance of the code. That being said, the code wasn't intended to be better performing than the SysFS interface, its main claim to merit is that SysFS GPIO is obsolete while GPIOD is the more future-proof interface. It just happens to implement some of the suggestions to filter out noise that have been raised in the past (like a HIGH_WAIT state) that do seem to work for some people (e.g. @trabant-asb ). |
|
Just to clarify, there is no shielding. The only changes are a different GPIO pin (because I have a twin connector so I need 3V3 or GND in the second GPIO row) and moving from pull-down/3V3 to pull-up/GND logic. The circuit is as simple as it gets otherwise: crimp connector on the Pi, the referenced unshielded twisted wire towards the sensor. No capacitors or shielding are present. |
|
Almost two weeks later I am happy to report that there are still no weird spikes from the lamp and the gas consumption is being monitored properly. |
|
Still no issues despite direct connection to GPIO and close proximity to an ancient CCFL tube. |
|
Still not an impulse too few or too many. Rock solid. |
|
thanks for the reminder. |
|
hey @r00t- I agree that this is not ready for mass inclusion at this stage. There were some comments in this thread regarding the code, e.g.: gpiochip is hardcoded to 0 and the appraoch to use gpioline > 1000 to configure the plugin is hacky default active high/low configuration is the inverse from the sysfs GPIO approach |
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.