Adding GPIO interface #1855
Merged
Adding GPIO interface #1855
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jimklimov
requested changes
Feb 26, 2023
There was a problem hiding this comment.
Thanks, quite a solid contribution - but some changes are desired, as commented in details.
- GPL headers in sources
- Make sure ASCIIDOC markup (man page) renders reasonably
- Complete the
configure.ac/m4/Makefile.amintegration (and a few bugs in it) - Coding style can be made friendlier :)
- Probably something else here and there...
- A
NEWSfile entry makes sense BTW, not just for a new driver but for a new category of drivers. - Probably
docs/configure.txtalso needs a paragraph for the new option.
docs/man/gpio.txt
Outdated
| file (/dev/gpiochip0 for example) to allow the run-time NUT driver user | ||
| account to access it, like by adding the following rule to rules.d directory: | ||
| SUBSYSTEM=="gpio*", PROGRAM="/bin/sh -c '\ | ||
| chown -R nut:nut /dev/gpiochip0 && chmod -R 700 /dev/gpiochip0\ |
There was a problem hiding this comment.
This code example should be indented, and possibly preceded by a blank line.
docs/man/gpio.txt
Outdated
| (|)operations are supported for now. nut_state should correspond to NUT | ||
| state, line_num to GPIO line number connected to UPS open collector pin. | ||
| CyberShield CSN27U12V describes pins as: | ||
| ON BATTERY Low when operating from utility line |
There was a problem hiding this comment.
I think asciidoc table markup (with pipe characters) would do well here, and this may need blank lines around to differentiate from other paragraphs. If possible, build with HTML docs enabled to iterate the result-oriented markup more easily (or use an IDE with an asciidoc plugin to render it real-time as you type) :)
docs/man/Makefile.am
Outdated
| @@ -669,6 +669,17 @@ endif | |||
|
|
|||
| HTML_LINUX_I2C_MANS = asem.html pijuice.html | |||
|
|
|||
| SRC_LINUX_GPIO_PAGES = gpio.txt | |||
There was a problem hiding this comment.
Just in case: is it right to call this a LINUX_GPIO? The currently used libgpiod "backend" does say it is for linux, but original discussion implied this driver could be more generic with other platforms logically supporting the interface.
|
|
||
| NUT_ARG_WITH([gpio], [build and install GPIO driver], [auto]) | ||
|
|
||
| dnl ${nut_with_gpio}: any value except "yes" or "no" is treated as "auto". |
There was a problem hiding this comment.
I think somewhere here it should check for libgpiod (potentially other equivalents later).
Typically the check is done as a new method under m4 directory (copy and adapt an existing file), setting the LIBGPIO_CFLAGS, LIBGPIO_LDFLAGS, etc. as appropriate, manually or from pkg-config data where available.
The configure.ac code calls such method, and if things are found and requested mode was "auto" it becomes "yes", otherwise "no". If the request was "yes" but library was not found, it should cause a fatal AC_MSG_ERROR.
|
|
||
| # distribute all drivers, even ones that are not built by default | ||
| EXTRA_PROGRAMS = $(SERIAL_DRIVERLIST) $(USB_DRIVERLIST) $(SERIAL_USB_DRIVERLIST) | ||
| EXTRA_PROGRAMS += $(SNMP_DRIVERLIST) $(NEONXML_DRIVERLIST) $(MACOSX_DRIVERLIST) | ||
| EXTRA_PROGRAMS += $(LINUX_I2C_DRIVERLIST) | ||
| EXTRA_PROGRAMS += $(NUTSW_DRIVERLIST) | ||
| EXTRA_PROGRAMS += $(GPIO_DRIVERLIST) |
There was a problem hiding this comment.
This was supposed to add a new line, not forget about software-only drivers, right? ;)
drivers/generic_gpio_common.h
Outdated
| #define GENERIC_GPIO_COMMON_H | ||
|
|
||
| #include <stdlib.h> | ||
| #include <gpiod.h> |
There was a problem hiding this comment.
This is technically #if HAVE_GPIOD_H or #if WITH_LIBGPIOD - right?
The file claims to be common, but hardcodes a dependency on (currently the only) one backend here.
| @@ -0,0 +1,48 @@ | |||
| #ifndef GENERIC_GPIO_COMMON_H | |||
|
|
||
| typedef struct rulesint_t { /* structure to store processed rules configuration per each state */ | ||
| char stateName[12]; /* NUT state name for rules in cRules */ | ||
| int archVal; /* previous state value */ |
There was a problem hiding this comment.
Just in case: does this deal with architecture-dependent sizes of int or some wire-protocol specific (u)int32_t etc.? The nut_stdint.h takes care of providing those for different platforms.
drivers/generic_gpio_libgpiod.c
Outdated
| * or docs/snmp-subdrivers.txt for SNMP devices | ||
| */ | ||
| /* ./configure --with-pidpath=/run/nut --with-altpidpath=/run/nut --with-statepath=/run/nut --sysconfdir=/etc/nut --with-gpio --with-user=nut --with-group=nut */ | ||
| #pragma GCC optimize("O0") |
There was a problem hiding this comment.
Similar comments to above - GPL header, this pragma, spaces in clauses (code style/readability).
drivers/Makefile.am
Outdated
| generic_gpio_libgpiod_SOURCES = generic_gpio_common.c generic_gpio_libgpiod.c | ||
| #gpio_CFLAGS = $(AM_CFLAGS) | ||
| #gpio_LDADD = $(LDADD_DRIVERS) -lgpiod | ||
| generic_gpio_libgpiod_LDADD = $(LDADD_DRIVERS) -lgpiod |
There was a problem hiding this comment.
I'm a bit split on whether explicit -lgpiod belongs here. Consistently, a LIBGPIOD_LIBS as detected by configure.ac and m4 snippet, would be better (especially if some more custom flags come later or on certain platforms).
Also, I think this block belongs lower in the file, among "non-serial drivers" - e.g. near I2C as the added lines neighbor elsewhere.
|
By the way, does GPIO availability on systemd-aware OSes rely on some certain units? Should |
|
As far as I know GPIO support is implemented via kernel driver - should
not depend systemd. Permissions may cause issues - will have to check. I
remember permission issues some long time ago, but have not seen
anything similar recently. Remember some changes in rules.d and then
stable start, but need to check once more.
…On 2/26/23 18:06, Jim Klimov wrote:
By the way, does GPIO availability on systemd-aware OSes rely on some
certain units? Should |nut-driver-enumerator| be updated to generate a
special dependency for these drivers to start as soon as (but not
sooner than) they are able to talk to hardware?
—
Reply to this email directly, view it on GitHub
<#1855 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AQO3GIPT3KZFJQDFRYGUA5TWZN5PXANCNFSM6AAAAAAVEPH5EM>.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
Clear on next steps.
published part of fixes: m4 will follow tomorrow.
Coding style - 1st round.
On 2/26/23 13:55, Jim Klimov wrote:
***@***.**** requested changes on this pull request.
Thanks, quite a solid contribution - but some changes are desired, as
commented in details.
* GPL headers in sources
* Make sure ASCIIDOC markup (man page) renders reasonably
* Complete the |configure.ac|/|m4|/|Makefile.am| integration (and a
few bugs in it)
* Coding style can be made friendlier :)
* Probably something else here and there
------------------------------------------------------------------------
In docs/man/gpio.txt
<#1855 (comment)>:
> +SHUTDOWN COMMAND
+----------------
+
+This driver does not support shutdown command.
+
+INSTALLATION
+------------
+
+This driver is not built by default. You can build it by installing
+libgpiod and running `configure --with-gpio=yes`.
+
+You also need to give proper permissions on the local serial device
+file (/dev/gpiochip0 for example) to allow the run-time NUT driver user
+account to access it, like by adding the following rule to rules.d directory:
+SUBSYSTEM=="gpio*", PROGRAM="/bin/sh -c '\
+ chown -R nut:nut /dev/gpiochip0 && chmod -R 700 /dev/gpiochip0\
This code example should be indented, and possibly preceded by a blank
line.
------------------------------------------------------------------------
In docs/man/gpio.txt
<#1855 (comment)>:
> +---------------
+
+This driver supports the following optional settings in the
+linkman:ups.conf[5] file:
+
+Driver control:
+~~~~~~~~~~~~~~
+
+*rules*='value'::
+A string consisting of sub-strings. Each sub-string describes GPIO line
+states conversion formula to specific NUT state, like
+nut_state=[^]line_num[logical_operation.line_num]. Not (^) , and (&) , or
+(|)operations are supported for now. nut_state should correspond to NUT
+state, line_num to GPIO line number connected to UPS open collector pin.
+CyberShield CSN27U12V describes pins as:
+ON BATTERY Low when operating from utility line
I think asciidoc table markup (with pipe characters) would do well
here, and this may need blank lines around to differentiate from other
paragraphs. If possible, build with HTML docs enabled to iterate the
result-oriented markup more easily (or use an IDE with an asciidoc
plugin to render it real-time as you type) :)
------------------------------------------------------------------------
In docs/man/Makefile.am
<#1855 (comment)>:
> @@ -669,6 +669,17 @@ endif
HTML_LINUX_I2C_MANS = asem.html pijuice.html
+SRC_LINUX_GPIO_PAGES = gpio.txt
Just in case: is it right to call this a LINUX_GPIO? The currently
used libgpiod "backend" does say it is for linux, but original
discussion implied this driver could be more generic with other
platforms logically supporting the interface.
Removed _LINUX to match agreement
------------------------------------------------------------------------
In configure.ac
<#1855 (comment)>:
> @@ -1526,6 +1534,20 @@ dnl and AM_CONDITIONALs (see below)...
AM_CONDITIONAL(WITH_FREEIPMI, test "${nut_with_freeipmi}" = "yes")
dnl AM_CONDITIONAL(WITH_OPENIPMI, test "${nut_with_openipmi}" = "yes")
+dnl ----------------------------------------------------------------------
+dnl Check for with-gpio
+
+nut_gpio_lib=""
+
+NUT_ARG_WITH([gpio], [build and install GPIO driver], [auto])
+
+dnl ${nut_with_gpio}: any value except "yes" or "no" is treated as "auto".
I think somewhere here it should check for libgpiod (potentially other
equivalents later).
Typically the check is done as a new method under |m4| directory (copy
and adapt an existing file), setting the |LIBGPIO_CFLAGS|,
|LIBGPIO_LDFLAGS|, etc. as appropriate, manually or from |pkg-config|
data where available.
The |configure.ac| code calls such method, and if things are found and
requested mode was "auto" it becomes "yes", otherwise "no". If the
request was "yes" but library was not found, it should cause a fatal
|AC_MSG_ERROR|.
------------------------------------------------------------------------
In drivers/Makefile.am
<#1855 (comment)>:
>
# distribute all drivers, even ones that are not built by default
EXTRA_PROGRAMS = $(SERIAL_DRIVERLIST) $(USB_DRIVERLIST) $(SERIAL_USB_DRIVERLIST)
EXTRA_PROGRAMS += $(SNMP_DRIVERLIST) $(NEONXML_DRIVERLIST) $(MACOSX_DRIVERLIST)
EXTRA_PROGRAMS += $(LINUX_I2C_DRIVERLIST)
-EXTRA_PROGRAMS += $(NUTSW_DRIVERLIST)
+EXTRA_PROGRAMS += $(GPIO_DRIVERLIST)
This was supposed to add a new line, not forget about software-only
drivers, right? ;)
------------------------------------------------------------------------
In drivers/generic_gpio_common.c
<#1855 (comment)>:
> @@ -0,0 +1,444 @@
+/*
+ anything commented is optional
+ anything else is mandatory
+
+ for more information, refer to:
+ * docs/developers.txt
+ * docs/new-drivers.txt
+ * docs/new-names.txt
+
+ and possibly also to:
+ * docs/hid-subdrivers.txt for USB/HID devices
+ * or docs/snmp-subdrivers.txt for SNMP devices
+*/
+/* ./configure --with-pidpath=/run/nut --with-altpidpath=/run/nut --with-statepath=/run/nut --sysconfdir=/etc/nut --with-gpio --with-user=nut --with-group=nut */
+#pragma GCC optimize("O0")
By release time this should go away (or hidden by |#if 0|) :)
Is this to ease debugging, or is there some compile/run-time issue
that the driver won't work because of optimizations? Then a comment to
keep this pragma long-term would help.
Removed - debugger did not stop at certain statements as they were
removed because of optimization.
------------------------------------------------------------------------
In drivers/generic_gpio_common.c
<#1855 (comment)>:
> @@ -0,0 +1,444 @@
+/*
Add the skeleton GPL header with your copyrights into the new source
files, please.
------------------------------------------------------------------------
In drivers/generic_gpio_common.c
<#1855 (comment)>:
> + upsfd->upsLinesCount=0;
+ upsfd->upsMaxLine=0;
+ for(i=0; i<upsfd->rulesCount; i++) {
+ for(j=0; j<upsfd->rules[i]->subCount; j++) {
+ int pinOnList=0;
+ for(k=0; k<upsfd->upsLinesCount && !pinOnList; k++) {
+ if(upsfd->upsLines[k]==upsfd->rules[i]->cRules[j]) {
+ pinOnList=1;
+ }
+ }
+ if(!pinOnList) {
+ if(upsfd->rules[i]->cRules[j]>=0) {
+ upsfd->upsLinesCount++;
+ upsfd->upsLines=xrealloc(upsfd->upsLines, sizeof(upsfd->upsLines[0])*upsfd->upsLinesCount);
+ upsfd->upsLines[upsfd->upsLinesCount-1]=upsfd->rules[i]->cRules[j];
+ if(upsfd->upsLines[upsfd->upsLinesCount-1]>upsfd->upsMaxLine) {
General coding style (all around the file) - this would be much more
readable with spaces around operators like |<|, |>|, |=|, |==| etc.,
and we have spaces between |if/for| and the |(| parenthesis after it :)
------------------------------------------------------------------------
In drivers/generic_gpio_common.h
<#1855 (comment)>:
> @@ -0,0 +1,48 @@
+#ifndef GENERIC_GPIO_COMMON_H
+#define GENERIC_GPIO_COMMON_H
+
+#include <stdlib.h>
+#include <gpiod.h>
This is technically |#if HAVE_GPIOD_H| or |#if WITH_LIBGPIOD| - right?
The file claims to be common, but hardcodes a dependency on (currently
the only) one backend here.
Moved to generic_gpio_libgpiod.h - missed change during refactoring and
now common part is fully library independent
------------------------------------------------------------------------
In drivers/generic_gpio_common.h
<#1855 (comment)>:
> @@ -0,0 +1,48 @@
+#ifndef GENERIC_GPIO_COMMON_H
GPL header
------------------------------------------------------------------------
In drivers/generic_gpio_common.h
<#1855 (comment)>:
> +
+/* rules commands definition */
+#define RULES_CMD_NOT -2
+#define RULES_CMD_AND -3
+#define RULES_CMD_OR -4
+#define RULES_CMD_OBR -5
+#define RULES_CMD_CBR -6
+#define RULES_CMD_LAST RULES_CMD_CBR
+
+/* run option definitions */
+#define ROPT_REQRES 0x00000001 /* reserve GPIO lines only during request processing */
+#define ROPT_EVMODE 0x00000002 /* event driven run */
+
+typedef struct rulesint_t { /* structure to store processed rules configuration per each state */
+ char stateName[12]; /* NUT state name for rules in cRules */
+ int archVal; /* previous state value */
Just in case: does this deal with architecture-dependent sizes of
|int| or some wire-protocol specific |(u)int32_t| etc.? The
|nut_stdint.h| takes care of providing those for different platforms.
No, plain integers, not related to wire or h/w.
… ------------------------------------------------------------------------
In drivers/generic_gpio_libgpiod.c
<#1855 (comment)>:
> @@ -0,0 +1,296 @@
+/*
+ anything commented is optional
+ anything else is mandatory
+
+ for more information, refer to:
+ * docs/developers.txt
+ * docs/new-drivers.txt
+ * docs/new-names.txt
+
+ and possibly also to:
+ * docs/hid-subdrivers.txt for USB/HID devices
+ * or docs/snmp-subdrivers.txt for SNMP devices
+*/
+/* ./configure --with-pidpath=/run/nut --with-altpidpath=/run/nut --with-statepath=/run/nut --sysconfdir=/etc/nut --with-gpio --with-user=nut --with-group=nut */
+#pragma GCC optimize("O0")
Similar comments to above - GPL header, this pragma, spaces in clauses
(code style/readability).
------------------------------------------------------------------------
In drivers/Makefile.am
<#1855 (comment)>:
> @@ -164,6 +171,11 @@ upscode2_LDADD = $(LDADD) -lm
victronups_SOURCES = victronups.c
riello_ser_SOURCES = riello.c riello_ser.c
riello_ser_LDADD = $(LDADD) -lm
+#gpio_SOURCES = generic_gpio_common.c generic_gpio_libgpiod.c
+generic_gpio_libgpiod_SOURCES = generic_gpio_common.c generic_gpio_libgpiod.c
+#gpio_CFLAGS = $(AM_CFLAGS)
+#gpio_LDADD = $(LDADD_DRIVERS) -lgpiod
+generic_gpio_libgpiod_LDADD = $(LDADD_DRIVERS) -lgpiod
I'm a bit split on whether explicit |-lgpiod| belongs here.
Consistently, a |LIBGPIOD_LIBS| as detected by |configure.ac| and |m4|
snippet, would be better (especially if some more custom flags come
later or on certain platforms).
Also, I think this block belongs lower in the file, among "non-serial
drivers" - e.g. near I2C as the added lines neighbor elsewhere.
—
Reply to this email directly, view it on GitHub
<#1855 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AQO3GIPUVBRHXDITNK5TVTLWZNAC7ANCNFSM6AAAAAAVEPH5EM>.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
m4 is in too, tested only on singe local machine.
…On 2/26/23 22:57, MODRIS BĒRZONIS wrote:
Clear on next steps.
published part of fixes: m4 will follow tomorrow.
Coding style - 1st round.
On 2/26/23 13:55, Jim Klimov wrote:
>
> ***@***.**** requested changes on this pull request.
>
> Thanks, quite a solid contribution - but some changes are desired, as
> commented in details.
>
> * GPL headers in sources
> * Make sure ASCIIDOC markup (man page) renders reasonably
> * Complete the |configure.ac|/|m4|/|Makefile.am| integration (and a
> few bugs in it)
> * Coding style can be made friendlier :)
> * Probably something else here and there
>
> ------------------------------------------------------------------------
>
> In docs/man/gpio.txt
> <#1855 (comment)>:
>
> > +SHUTDOWN COMMAND
> +----------------
> +
> +This driver does not support shutdown command.
> +
> +INSTALLATION
> +------------
> +
> +This driver is not built by default. You can build it by installing
> +libgpiod and running `configure --with-gpio=yes`.
> +
> +You also need to give proper permissions on the local serial device
> +file (/dev/gpiochip0 for example) to allow the run-time NUT driver user
> +account to access it, like by adding the following rule to rules.d directory:
> +SUBSYSTEM=="gpio*", PROGRAM="/bin/sh -c '\
> + chown -R nut:nut /dev/gpiochip0 && chmod -R 700 /dev/gpiochip0\
>
> This code example should be indented, and possibly preceded by a
> blank line.
>
> ------------------------------------------------------------------------
>
> In docs/man/gpio.txt
> <#1855 (comment)>:
>
> > +---------------
> +
> +This driver supports the following optional settings in the
> +linkman:ups.conf[5] file:
> +
> +Driver control:
> +~~~~~~~~~~~~~~
> +
> +*rules*='value'::
> +A string consisting of sub-strings. Each sub-string describes GPIO line
> +states conversion formula to specific NUT state, like
> +nut_state=[^]line_num[logical_operation.line_num]. Not (^) , and (&) , or
> +(|)operations are supported for now. nut_state should correspond to NUT
> +state, line_num to GPIO line number connected to UPS open collector pin.
> +CyberShield CSN27U12V describes pins as:
> +ON BATTERY Low when operating from utility line
>
> I think asciidoc table markup (with pipe characters) would do well
> here, and this may need blank lines around to differentiate from
> other paragraphs. If possible, build with HTML docs enabled to
> iterate the result-oriented markup more easily (or use an IDE with an
> asciidoc plugin to render it real-time as you type) :)
>
> ------------------------------------------------------------------------
>
> In docs/man/Makefile.am
> <#1855 (comment)>:
>
> > @@ -669,6 +669,17 @@ endif
>
> HTML_LINUX_I2C_MANS = asem.html pijuice.html
>
> +SRC_LINUX_GPIO_PAGES = gpio.txt
>
> Just in case: is it right to call this a LINUX_GPIO? The currently
> used libgpiod "backend" does say it is for linux, but original
> discussion implied this driver could be more generic with other
> platforms logically supporting the interface.
>
Removed _LINUX to match agreement
> ------------------------------------------------------------------------
>
> In configure.ac
> <#1855 (comment)>:
>
> > @@ -1526,6 +1534,20 @@ dnl and AM_CONDITIONALs (see below)...
> AM_CONDITIONAL(WITH_FREEIPMI, test "${nut_with_freeipmi}" = "yes")
> dnl AM_CONDITIONAL(WITH_OPENIPMI, test "${nut_with_openipmi}" = "yes")
>
> +dnl ----------------------------------------------------------------------
> +dnl Check for with-gpio
> +
> +nut_gpio_lib=""
> +
> +NUT_ARG_WITH([gpio], [build and install GPIO driver], [auto])
> +
> +dnl ${nut_with_gpio}: any value except "yes" or "no" is treated as "auto".
>
> I think somewhere here it should check for libgpiod (potentially
> other equivalents later).
>
> Typically the check is done as a new method under |m4| directory
> (copy and adapt an existing file), setting the |LIBGPIO_CFLAGS|,
> |LIBGPIO_LDFLAGS|, etc. as appropriate, manually or from |pkg-config|
> data where available.
>
> The |configure.ac| code calls such method, and if things are found
> and requested mode was "auto" it becomes "yes", otherwise "no". If
> the request was "yes" but library was not found, it should cause a
> fatal |AC_MSG_ERROR|.
>
> ------------------------------------------------------------------------
>
> In drivers/Makefile.am
> <#1855 (comment)>:
>
> >
> # distribute all drivers, even ones that are not built by default
> EXTRA_PROGRAMS = $(SERIAL_DRIVERLIST) $(USB_DRIVERLIST) $(SERIAL_USB_DRIVERLIST)
> EXTRA_PROGRAMS += $(SNMP_DRIVERLIST) $(NEONXML_DRIVERLIST) $(MACOSX_DRIVERLIST)
> EXTRA_PROGRAMS += $(LINUX_I2C_DRIVERLIST)
> -EXTRA_PROGRAMS += $(NUTSW_DRIVERLIST)
> +EXTRA_PROGRAMS += $(GPIO_DRIVERLIST)
>
> This was supposed to add a new line, not forget about software-only
> drivers, right? ;)
>
> ------------------------------------------------------------------------
>
> In drivers/generic_gpio_common.c
> <#1855 (comment)>:
>
> > @@ -0,0 +1,444 @@
> +/*
> + anything commented is optional
> + anything else is mandatory
> +
> + for more information, refer to:
> + * docs/developers.txt
> + * docs/new-drivers.txt
> + * docs/new-names.txt
> +
> + and possibly also to:
> + * docs/hid-subdrivers.txt for USB/HID devices
> + * or docs/snmp-subdrivers.txt for SNMP devices
> +*/
> +/* ./configure --with-pidpath=/run/nut --with-altpidpath=/run/nut --with-statepath=/run/nut --sysconfdir=/etc/nut --with-gpio --with-user=nut --with-group=nut */
> +#pragma GCC optimize("O0")
>
> By release time this should go away (or hidden by |#if 0|) :)
>
> Is this to ease debugging, or is there some compile/run-time issue
> that the driver won't work because of optimizations? Then a comment
> to keep this pragma long-term would help.
>
Removed - debugger did not stop at certain statements as they were
removed because of optimization.
> ------------------------------------------------------------------------
>
> In drivers/generic_gpio_common.c
> <#1855 (comment)>:
>
> > @@ -0,0 +1,444 @@
> +/*
>
> Add the skeleton GPL header with your copyrights into the new source
> files, please.
>
> ------------------------------------------------------------------------
>
> In drivers/generic_gpio_common.c
> <#1855 (comment)>:
>
> > + upsfd->upsLinesCount=0;
> + upsfd->upsMaxLine=0;
> + for(i=0; i<upsfd->rulesCount; i++) {
> + for(j=0; j<upsfd->rules[i]->subCount; j++) {
> + int pinOnList=0;
> + for(k=0; k<upsfd->upsLinesCount && !pinOnList; k++) {
> + if(upsfd->upsLines[k]==upsfd->rules[i]->cRules[j]) {
> + pinOnList=1;
> + }
> + }
> + if(!pinOnList) {
> + if(upsfd->rules[i]->cRules[j]>=0) {
> + upsfd->upsLinesCount++;
> + upsfd->upsLines=xrealloc(upsfd->upsLines, sizeof(upsfd->upsLines[0])*upsfd->upsLinesCount);
> + upsfd->upsLines[upsfd->upsLinesCount-1]=upsfd->rules[i]->cRules[j];
> + if(upsfd->upsLines[upsfd->upsLinesCount-1]>upsfd->upsMaxLine) {
>
> General coding style (all around the file) - this would be much more
> readable with spaces around operators like |<|, |>|, |=|, |==| etc.,
> and we have spaces between |if/for| and the |(| parenthesis after it :)
>
> ------------------------------------------------------------------------
>
> In drivers/generic_gpio_common.h
> <#1855 (comment)>:
>
> > @@ -0,0 +1,48 @@
> +#ifndef GENERIC_GPIO_COMMON_H
> +#define GENERIC_GPIO_COMMON_H
> +
> +#include <stdlib.h>
> +#include <gpiod.h>
>
> This is technically |#if HAVE_GPIOD_H| or |#if WITH_LIBGPIOD| - right?
> The file claims to be common, but hardcodes a dependency on
> (currently the only) one backend here.
>
Moved to generic_gpio_libgpiod.h - missed change during refactoring
and now common part is fully library independent
> ------------------------------------------------------------------------
>
> In drivers/generic_gpio_common.h
> <#1855 (comment)>:
>
> > @@ -0,0 +1,48 @@
> +#ifndef GENERIC_GPIO_COMMON_H
>
> GPL header
>
> ------------------------------------------------------------------------
>
> In drivers/generic_gpio_common.h
> <#1855 (comment)>:
>
> > +
> +/* rules commands definition */
> +#define RULES_CMD_NOT -2
> +#define RULES_CMD_AND -3
> +#define RULES_CMD_OR -4
> +#define RULES_CMD_OBR -5
> +#define RULES_CMD_CBR -6
> +#define RULES_CMD_LAST RULES_CMD_CBR
> +
> +/* run option definitions */
> +#define ROPT_REQRES 0x00000001 /* reserve GPIO lines only during request processing */
> +#define ROPT_EVMODE 0x00000002 /* event driven run */
> +
> +typedef struct rulesint_t { /* structure to store processed rules configuration per each state */
> + char stateName[12]; /* NUT state name for rules in cRules */
> + int archVal; /* previous state value */
>
> Just in case: does this deal with architecture-dependent sizes of
> |int| or some wire-protocol specific |(u)int32_t| etc.? The
> |nut_stdint.h| takes care of providing those for different platforms.
>
No, plain integers, not related to wire or h/w.
> ------------------------------------------------------------------------
>
> In drivers/generic_gpio_libgpiod.c
> <#1855 (comment)>:
>
> > @@ -0,0 +1,296 @@
> +/*
> + anything commented is optional
> + anything else is mandatory
> +
> + for more information, refer to:
> + * docs/developers.txt
> + * docs/new-drivers.txt
> + * docs/new-names.txt
> +
> + and possibly also to:
> + * docs/hid-subdrivers.txt for USB/HID devices
> + * or docs/snmp-subdrivers.txt for SNMP devices
> +*/
> +/* ./configure --with-pidpath=/run/nut --with-altpidpath=/run/nut --with-statepath=/run/nut --sysconfdir=/etc/nut --with-gpio --with-user=nut --with-group=nut */
> +#pragma GCC optimize("O0")
>
> Similar comments to above - GPL header, this pragma, spaces in
> clauses (code style/readability).
>
> ------------------------------------------------------------------------
>
> In drivers/Makefile.am
> <#1855 (comment)>:
>
> > @@ -164,6 +171,11 @@ upscode2_LDADD = $(LDADD) -lm
> victronups_SOURCES = victronups.c
> riello_ser_SOURCES = riello.c riello_ser.c
> riello_ser_LDADD = $(LDADD) -lm
> +#gpio_SOURCES = generic_gpio_common.c generic_gpio_libgpiod.c
> +generic_gpio_libgpiod_SOURCES = generic_gpio_common.c generic_gpio_libgpiod.c
> +#gpio_CFLAGS = $(AM_CFLAGS)
> +#gpio_LDADD = $(LDADD_DRIVERS) -lgpiod
> +generic_gpio_libgpiod_LDADD = $(LDADD_DRIVERS) -lgpiod
>
> I'm a bit split on whether explicit |-lgpiod| belongs here.
> Consistently, a |LIBGPIOD_LIBS| as detected by |configure.ac| and
> |m4| snippet, would be better (especially if some more custom flags
> come later or on certain platforms).
>
> Also, I think this block belongs lower in the file, among "non-serial
> drivers" - e.g. near I2C as the added lines neighbor elsewhere.
>
> —
> Reply to this email directly, view it on GitHub
> <#1855 (review)>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AQO3GIPUVBRHXDITNK5TVTLWZNAC7ANCNFSM6AAAAAAVEPH5EM>.
> You are receiving this because you authored the thread.Message ID:
> ***@***.***>
>
|
|
I checked logs on running system: and don't see recent cases with
permission failures that might be caused slow rules.d execution. Looks I
did safety change in systemd
configuration for nut-server.service by adding restart option for it, like
[Unit]
Description=Network UPS Tools - power devices information server
After=local-fs.target network.target nut-driver.service
# We don't Require drivers to be successfully started! This would be
# a change of behavior compared to init SysV, and could prevent from
# accessing successfully started, at least to audit a system.
Wants=nut-driver.service
Before=nut-monitor.service
[Service]
ExecStart=/sbin/upsd
Type=forking
Restart=on-failure
RestartSec=10
[Install]
WantedBy=multi-user.target
This ensures that driver is reloaded in case of chip open failure.
Modris
…On 2/26/23 21:11, MODRIS BĒRZONIS wrote:
As far as I know GPIO support is implemented via kernel driver -
should not depend systemd. Permissions may cause issues - will have to
check. I remember permission issues some long time ago, but have not
seen anything similar recently. Remember some changes in rules.d and
then stable start, but need to check once more.
On 2/26/23 18:06, Jim Klimov wrote:
>
> By the way, does GPIO availability on systemd-aware OSes rely on some
> certain units? Should |nut-driver-enumerator| be updated to generate
> a special dependency for these drivers to start as soon as (but not
> sooner than) they are able to talk to hardware?
>
> —
> Reply to this email directly, view it on GitHub
> <#1855 (comment)>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AQO3GIPT3KZFJQDFRYGUA5TWZN5PXANCNFSM6AAAAAAVEPH5EM>.
> You are receiving this because you authored the thread.Message ID:
> ***@***.***>
>
|
|
Confused about latest comment: the Also, you should not restart the data server (upsd) for hiccups with a driver. Driver may die, restart and reconnect to upsd. |
jimklimov
added
feature
GPIO
|
Version is 2.7.4 for package, distro version.
The latest version already has systemd fixes, that solves possibly late
execution of rules.d. We don't need other changes beyond driver itself.
…On 2/28/23 14:34, Jim Klimov wrote:
Confused about latest comment: the |nut-server.service.in| in NUT
sources includes the |Restart| option and a very different text (more
comments, etc.) Is the version you have deployed coming from a recent
build, or from NUT 2.7.4 packages by distro?
Also, you should not restart the data server (upsd) for hiccups with a
driver. Driver may die, restart and reconnect to upsd.
—
Reply to this email directly, view it on GitHub
<#1855 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AQO3GIPKDHWZZLKT6IO5AQLWZXWEVANCNFSM6AAAAAAVEPH5EM>.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
Code is finalized from my side.
What about unit test code? I don't see how to store/run related code.
Wanted to add some cases for rules processing test...
…On 2/28/23 14:34, Jim Klimov wrote:
Confused about latest comment: the |nut-server.service.in| in NUT
sources includes the |Restart| option and a very different text (more
comments, etc.) Is the version you have deployed coming from a recent
build, or from NUT 2.7.4 packages by distro?
Also, you should not restart the data server (upsd) for hiccups with a
driver. Driver may die, restart and reconnect to upsd.
—
Reply to this email directly, view it on GitHub
<#1855 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AQO3GIPKDHWZZLKT6IO5AQLWZXWEVANCNFSM6AAAAAAVEPH5EM>.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
Unfortunately so far tests are rather nascent. There are a few one-off test programs for some aspects and libraries, but no sturdy framework to rule them all. Check under |
…lways redistributed
…arname clash (in tests)
…avoid local/global varname clashes for chipName
…clash with global varname in main.c (due to tests/generic_gpio_utest.c construction)
… the original main() from included main.c
|
need to take out main ... if use library then getting message about
duplicate main, selected include as no messages in this case. May be
there is another solution too, but this works.
…On 3/13/23 13:24, Jim Klimov wrote:
Hm, includes of C source files?.. that was a very unorthodox approach :)
—
Reply to this email directly, view it on GitHub
<#1855 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AQO3GILZKOXGD466XJPXXZ3W337WHANCNFSM6AAAAAAVEPH5EM>.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
… out-of-tree builds the generic_gpio_test.txt resource file
|
Well, actually this needed a lot of chizeling around the rough edges to work and not spew warnings, pass distcheck, etc. ;) Normally C sources are built standalone and then linked together into a binary, not included directly - more so for testing of what should be library code. Was there a particular need to use much of I think something better is possible along the lines of |
… (and DRIVERS_MAIN_WITHOUT_MAIN)
…" and "dstate.c" right into test code
…_gpio_common.c generic_gpio_liblocal.c generic_gpio_libgpiod.c" in test code
|
At least on one system, these recent commits got rid of unidiomatic |
|
gpio test code is more close to unit testing rather than integration,
therefore supposing to have special code to execute selected portions of
code.
gpio tests runs into 3 separate areas:
1) generic driver test which includes rules testing and rules evaluation
parts;
2) library interface test for libgpiod implementation. This includes
mock library for libgpiod and requires part of drivers/main.c and
dstate.c. These tests verifies how update_ups_states , upsdrv_initups,
upsdrv_cleanup, upsdrv_makevartable, upsdrv_initinfo, upsdrv_updateinfo,
upsdrv_cleanup works. There are reason to run these tests just once per
source code changes (both driver/main.c&Co and gpio). I don't see
benefit to have separate library without main unless there are plans to
add some similar as gpio tests for other types of UPSes. Approach with
includes takes recent sources and tests them, no reason to keep interim
results or rely on delivery packages. We may run them even on any commit
just to prevent code brakes.
…On 3/13/23 15:11, Jim Klimov wrote:
Well, actually this needed a lot of chizeling around the rough edges
to work and not spew warnings, pass distcheck, etc. ;)
Normally C sources are built standalone and then linked together into
a binary, not included directly - more so for testing of what should
be library code. Was there a particular need to use much of
|drivers/main.c|, and the |dstate.c| directly in the test, as if part
of its source, rather than via linking? Was it due to testing of rules
support?
I think something better is possible along the lines of
|drivers/Makefile.am:libdummy_la_SOURCES = main.c dstate.c|, perhaps
instrumenting |drivers/main.c| to wrap |main()| into some |#ifndef
MAIN_WITHOUT_MAIN| sort of flag to build an object file that has
almost everything.
—
Reply to this email directly, view it on GitHub
<#1855 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AQO3GIPPZZFL2JCBUYDQQCDW34MJZANCNFSM6AAAAAAVEPH5EM>.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
there could be issue with access to
vartab_free(); vartab_h = NULL;
needed to clean up variables between test cases, both are static and
accessible when include
…On 3/13/23 16:12, Jim Klimov wrote:
At least on one system, these recent commits got rid of unidiomatic
|#include "somefile.c"| and passed for in-tree and out-of-tree
(distcheck) builds. Hoping CI likes it :)
—
Reply to this email directly, view it on GitHub
<#1855 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AQO3GIMBMUVJYYH3HITSN5TW34TMXANCNFSM6AAAAAAVEPH5EM>.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
then if we do changes in main.c may change vartab_free/vartab_h external
for testing build
…On 3/13/23 16:59, MODRIS BĒRZONIS wrote:
there could be issue with access to
vartab_free(); vartab_h = NULL;
needed to clean up variables between test cases, both are static and
accessible when include
On 3/13/23 16:12, Jim Klimov wrote:
>
> At least on one system, these recent commits got rid of unidiomatic
> |#include "somefile.c"| and passed for in-tree and out-of-tree
> (distcheck) builds. Hoping CI likes it :)
>
> —
> Reply to this email directly, view it on GitHub
> <#1855 (comment)>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AQO3GIMBMUVJYYH3HITSN5TW34TMXANCNFSM6AAAAAAVEPH5EM>.
> You are receiving this because you authored the thread.Message ID:
> ***@***.***>
>
|
|
Take a look in the recent commits' proposal of The Makefile changes, now explicitly referring to sources that were previously included, take care of building the test once per change of any source involved (previously this was not guaranteed - only that a change of timestamp of And indeed, having the |
|
Changes look fine. Tests pass for me on PC, will check on PI too, but
this will take couple hours.
…On 3/13/23 17:23, Jim Klimov wrote:
Take a look in the recent commits' proposal of |ifdef
DRIVERS_MAIN_WITHOUT_MAIN| - I got to a similar conclusion about
|static| so use of the keyword became conditional, depending on build
circumstances. This macro is defined for the test-program builds, and
so these test builds declare the methods and variables non-static (and
the new helper library exposes them for the linker).
The Makefile changes, now explicitly referring to sources that were
previously included, take care of building the test once per change of
any source involved (previously this was not guaranteed - only that a
change of timestamp of |generic_gpio_utest.c| would trigger rebuild of
the test program) as well as building the same source into different
object files with different settings (with the macro for test, without
it for driver).
And indeed, having the |libdummy_mockdrv.la| now allows easier
creation of more test programs that behave similarly to a real driver.
—
Reply to this email directly, view it on GitHub
<#1855 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AQO3GILVANADEM2QV2FVJSLW343VZANCNFSM6AAAAAAVEPH5EM>.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
Orange PI looks good too.
…On 3/13/23 18:17, MODRIS BĒRZONIS wrote:
Changes look fine. Tests pass for me on PC, will check on PI too, but
this will take couple hours.
On 3/13/23 17:23, Jim Klimov wrote:
>
> Take a look in the recent commits' proposal of |ifdef
> DRIVERS_MAIN_WITHOUT_MAIN| - I got to a similar conclusion about
> |static| so use of the keyword became conditional, depending on build
> circumstances. This macro is defined for the test-program builds, and
> so these test builds declare the methods and variables non-static
> (and the new helper library exposes them for the linker).
>
> The Makefile changes, now explicitly referring to sources that were
> previously included, take care of building the test once per change
> of any source involved (previously this was not guaranteed - only
> that a change of timestamp of |generic_gpio_utest.c| would trigger
> rebuild of the test program) as well as building the same source into
> different object files with different settings (with the macro for
> test, without it for driver).
>
> And indeed, having the |libdummy_mockdrv.la| now allows easier
> creation of more test programs that behave similarly to a real driver.
>
> —
> Reply to this email directly, view it on GitHub
> <#1855 (comment)>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AQO3GILVANADEM2QV2FVJSLW343VZANCNFSM6AAAAAAVEPH5EM>.
> You are receiving this because you authored the thread.Message ID:
> ***@***.***>
>
|
…for sources of tests are always redistributed
|
@modrisb : Thank you very much for this improvement! :) |
|
It was interesting activity, and you did a lot of improvements in making
it ready for release, thanks!
…On 3/15/23 09:38, Jim Klimov wrote:
@modrisb <https://github.com/modrisb> : Thank you very much for this
improvement! :)
—
Reply to this email directly, view it on GitHub
<#1855 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AQO3GIKMONOOCTLDTTHDVZDW4FWYBANCNFSM6AAAAAAVEPH5EM>.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Attempt to add support for UPSes using only open collector interface to report device state by connecting open collector pins to GPIO lines. Pins statuses are transformed to NUT states by configuration rules and is is device independent. Testing done on
CyberPower CyberShield CSN27U12V connected to Orange Pi Zero. Implementation uses libgpiod library to read GPIO lines statuses and does not change any other NUT components except build related.
General points
Described the changes in the PR submission or a separate issue, e.g.
known published or discovered protocols, applicable hardware (expected
compatible and actually tested/developed against), limitations, etc.
There may be multiple commits in the PR, aligned and commented with
a functional change. Notably, coding style changes better belong in a
separate PR, but certainly in a dedicated commit to simplify reviews
of "real" changes in the other commits. Similarly for typo fixes in
comments or text documents.
Frequent "underwater rocks" for driver addition/update PRs
Revised existing driver families and added a sub-driver if applicable
(
nutdrv_qx,usbhid-ups...) or added a brand new driver in the othercase.
Did not extend obsoleted drivers with new hardware support features
(notably
blazerand other single-device family drivers for Qx protocols,except the new
nutdrv_qxwhich should cover them all).For updated existing device drivers, bumped the
DRIVER_VERSIONmacroor its equivalent.
For USB devices (HID or not), revised that the driver uses unique
VID/PID combinations, or raised discussions when this is not the case
(several vendors do use same interface chips for unrelated protocols).
For new USB devices, built and committed the changes for the
scripts/upower/95-upower-hid.hwdbfileProposed NUT data mapping is aligned with existing
docs/nut-names.txtfile. If the device exposes useful data points not listed in the file, the
experimental.*namespace can be used as documented there, and discussionshould be raised on the NUT Developers mailing list to standardize the new
concept.
Updated
data/driver.list.inif applicable (new tested device info)Frequent "underwater rocks" for general C code PRs
structure layout and alignment in memory, endianness (layout of bytes and
bits in memory for multi-byte numeric types), or use of generic
intwherelanguage or libraries dictate the use of
size_t(orssize_tsometimes).Progress and errors are handled with
upsdebugx(),upslogx(),fatalx()and related methods, not with directprintf()orexit().Similarly, NUT helpers are used for error-checked memory allocation and
string operations (except where customized error handling is needed,
such as unlocking device ports, etc.)
Coding style (including whitespace for indentations) follows precedent
in the code of the file, and examples/guide in
docs/developers.txtfile.For newly added files, the
Makefile.amrecipes were updated and themake distchecktarget passes.General documentation updates
Updated
docs/acknowledgements.txt(for vendor-backed device support)Added or updated manual page information in
docs/man/*.txtfilesand corresponding recipe lists in
docs/man/Makefile.amfor new pagesPassed
make spellcheck, updated spell-checking dictionary in thedocs/nut.dictfile if needed (did not remove any words -- themakerule printout in case of changes suggests how to maintain it).
Additional work may be needed after posting this PR
Propose a PR for NUT DDL with detailed device data dumps from tests
against real hardware (the more models, the better).
Address NUT CI farm build failures for the PR: testing on numerous
platforms and toolkits can expose issues not seen on just one system.
the changed codebase.