Documentation for line_config PULL_UP, effect on line_event edges and line_request values?

Documentation for line_config PULL_UP, effect on line_event edges and line_request values?

[David C. Rankin]

All,

   First post, so only shoot me once if the rational is documented somewhere. 
I've been working with the gpio_v2 uABI (fantastic piece of work), but I've 
run into a very confusing combination using gpio_line_get_values where the 
line is PULL_UP makes going down "RISING" and going up "FALLING" and a .bits 
value of 1 is for zero voltage and a value of 0 for normal line voltage. I 
think I've digested it correctly, but I'm wondering if there is a better way 
to handle this and whether the chardev.html doc should be updated to further 
explain this behavior?

   The confusing part comes from what is defined as "active" and what is 
defined as "inactive" when the line is active low, e.g.

  /* gpio_v2 line config, line request and line values, read defaults set */
   struct gpio_v2_line_config linecfg = {
                               .flags =  GPIO_V2_LINE_FLAG_ACTIVE_LOW      |
                                         GPIO_V2_LINE_FLAG_INPUT           |
                                         GPIO_V2_LINE_FLAG_EDGE_RISING     |
                                         GPIO_V2_LINE_FLAG_EDGE_FALLING    |
                                         GPIO_V2_LINE_FLAG_BIAS_PULL_UP,
                               .num_attrs = 1 };

   In this case the configured line_request is passed to a thread for reading 
where the interest is in both the edges and the line_values.

   The documentation at 
https://docs.kernel.org/userspace-api/gpio/chardev.html is helpful, but it is 
silent on how the ...FLAG_ACTIVE_LOW/...PULL_UP basically inverts everything 
so that catching the ...FLAG_EDGE_RISING is actually the edge going from 
normal line voltage to zero (normally the falling edge of a waveform), how the 
value retrieved by gpio_line_get_values() then reports bit Hi (1) for the zero 
voltage state. The ...FLAG_EDGE_FALLING is then triggered when voltage goes 
from zero back to normal line voltage (normally the rising edge) and the value 
reported by gpio_line_get_values() is then Lo (0) at line voltage.

   The header gpio.h does provide more help. There the description of the 
attribute flags does indicate that rising will trigger on transition from 
(inactive to active) edges and falling will trigger on (active to inactive) 
edges, e.g.

/**
  * enum gpio_v2_line_flag - &struct gpio_v2_line_attribute.flags values
  ...
  * @GPIO_V2_LINE_FLAG_ACTIVE_LOW: line active state is physical low
  ...

  * @GPIO_V2_LINE_FLAG_EDGE_RISING: line detects rising (inactive to active)
  * edges
  * @GPIO_V2_LINE_FLAG_EDGE_FALLING: line detects falling (active to
  * inactive) edges
  ...

   Where there is a little ambiguity is in the comment for gpio_v2_line_values 
related to active/inactive .bits values. Taken together with the flags comment 
it's reasonably clear that active/inactive are as used in flags and not as 
commonly used (e.g. inactive - zero - low, active - non-zero - high). The 
comment reads:

/**
  * struct gpio_v2_line_values - Values of GPIO lines
  * @bits: a bitmap containing the value of the lines, set to 1 for active
  * and 0 for inactive.
  ...

   To make sure I was interpreting "active"/"inactive" and the effect on what 
is RISING and FALLING edge and .bits values I wrote a short program for the 
Raspberry Pi to catch the edges and values on button press and release and 
display the results. The results were indeed that the active RISING edge was 
the transition from line-voltage to zero, with a .bits value of 1 (Hi) for 
voltage zero, and on button release the inactive FALLING edge was the 
transition from zero to line-voltage with a .bits value of 0 (Lo) for line 
voltage.

   (if interested the short test program and Makefile for the Pi are at 
https://github.com/drankinatty/pi-wo-root/tree/master/tst/gpio_v2_button_value)

   My questions are:

  1. is there any thread or document squirreled away that contains a 
discussion of how this rational was arrived at?

  2. should the documentation at 
https://docs.kernel.org/userspace-api/gpio/chardev.html be updated to add the 
behavior for "active"/"inactive" and what flag this is dependent on (PULL_UP, 
ACTIVE_LOW or BOTH?) If so, I'd be glad to take a crack at the write-up and 
pass it to whoever for review and revision. (just let me know who the right 
person is to send it to if interested) The chardev.html seems like the right 
place for it rather than having to also locate and read gpio.h to find 
"active" and "inactive" (especially for newer users using latest libgpio for 
Pi, etc.. based on gpio_v2)

  3. is the expected programming approach to query the line config so that the 
.bits values can be interpreted as either 1 (Hi or Lo), or 0 (Lo or Hi)?

  (I guess that was where the biggest confusion was -- that a .bits value 0 
didn't mean no voltage, and vice-versa)

   Don't take this as a knock on the implementation, I think gpio_v2 is a 
stroke of genius, especially the debounce, but rather these were the parts 
that really were a bit difficult to suss out of the documentation and it may 
be helpful to include further explanation in the chardev.html pages explaining 
this a bit further.

NOTE Also:

   Links for the lists in 
https://docs.kernel.org/process/submitting-patches.html under the "Select the 
recipients for your patch" heading still point to 
http://vger.kernel.org/vger-lists.html (Majordomo)

-- 
David C. Rankin, J.D.,P.E.

Re: Documentation for line_config PULL_UP, effect on line_event edges and line_request values?

[Kent Gibson]

On Mon, Jun 03, 2024 at 05:48:05PM -0500, David C. Rankin wrote:
> All,
>

Well hello again.

>   First post, so only shoot me once if the rational is documented somewhere.

rationale

> I've been working with the gpio_v2 uABI (fantastic piece of work), but I've
> run into a very confusing combination using gpio_line_get_values where the
> line is PULL_UP makes going down "RISING" and going up "FALLING" and a .bits
> value of 1 is for zero voltage and a value of 0 for normal line voltage. I
> think I've digested it correctly, but I'm wondering if there is a better way
> to handle this and whether the chardev.html doc should be updated to further
> explain this behavior?
>

Quite possibly - it is easy to overlook what you consider obvious when
documenting, like line values being logical, not physical.  That seems
to be the key point you are missing.

>   The confusing part comes from what is defined as "active" and what is
> defined as "inactive" when the line is active low, e.g.
>

Line values are logical, and the documentation uses the terminology
active/inactive to emphasise this throughout.  That applies to getting
and setting values, and the polarity of edges.  You will note the
documentation never uses the hi/lo terminology - as that indicates physical
values.

OTOH bias (pull) is always physical.  I've never seen it used otherwise.
A pull-up is always a bias towards Vdd, and a pull-down is a bias towards GND.
That is how it is defined in hardware, and that is independent of whether
a line is considered active low.

The ACTIVE_LOW flag controls the mapping between physical and logical.
When set, the logical is the inverse of the physical.  When cleared
physical and logical are the same.
The ACTIVE_LOW flag does not change how bias is interpreted - those are
always physical.  Having bias also change polarity would be even more
confusing - have you ever seen a pull-up on a schematic referred to as a
pull-down because the line is active low?

>  /* gpio_v2 line config, line request and line values, read defaults set */
>   struct gpio_v2_line_config linecfg = {
>                               .flags =  GPIO_V2_LINE_FLAG_ACTIVE_LOW      |
>                                         GPIO_V2_LINE_FLAG_INPUT           |
>                                         GPIO_V2_LINE_FLAG_EDGE_RISING     |
>                                         GPIO_V2_LINE_FLAG_EDGE_FALLING    |
>                                         GPIO_V2_LINE_FLAG_BIAS_PULL_UP,
>                               .num_attrs = 1 };
>
>   In this case the configured line_request is passed to a thread for reading
> where the interest is in both the edges and the line_values.
>
>   The documentation at
> https://docs.kernel.org/userspace-api/gpio/chardev.html is helpful, but it
> is silent on how the ...FLAG_ACTIVE_LOW/...PULL_UP basically inverts

It is just the ACTIVE_LOW flag.  And as you quote below, when the
ACTIVE_LOW flag is set "line active state is physical low".

Btw, if you are going to provide links then use references, with the link
itself at the bottom of your email e.g.[1].

> everything so that catching the ...FLAG_EDGE_RISING is actually the edge
> going from normal line voltage to zero (normally the falling edge of a
> waveform), how the value retrieved by gpio_line_get_values() then reports
> bit Hi (1) for the zero voltage state. The ...FLAG_EDGE_FALLING is then
> triggered when voltage goes from zero back to normal line voltage (normally
> the rising edge) and the value reported by gpio_line_get_values() is then Lo
> (0) at line voltage.
>

Does highlighting that line values are logical help?

>   The header gpio.h does provide more help. There the description of the
> attribute flags does indicate that rising will trigger on transition from
> (inactive to active) edges and falling will trigger on (active to inactive)
> edges, e.g.
>
> /**
>  * enum gpio_v2_line_flag - &struct gpio_v2_line_attribute.flags values
>  ...
>  * @GPIO_V2_LINE_FLAG_ACTIVE_LOW: line active state is physical low
>  ...

Yup, when that flag is set logical is the inverse of physical.
Not clear enough?

>
>  * @GPIO_V2_LINE_FLAG_EDGE_RISING: line detects rising (inactive to active)
>  * edges
>  * @GPIO_V2_LINE_FLAG_EDGE_FALLING: line detects falling (active to
>  * inactive) edges
>  ...

So that does not makes it clear that the edge definitions are based on
logical values?

>
>   Where there is a little ambiguity is in the comment for
> gpio_v2_line_values related to active/inactive .bits values. Taken together
> with the flags comment it's reasonably clear that active/inactive are as
> used in flags and not as commonly used (e.g. inactive - zero - low, active -
> non-zero - high). The comment reads:
>
> /**
>  * struct gpio_v2_line_values - Values of GPIO lines
>  * @bits: a bitmap containing the value of the lines, set to 1 for active
>  * and 0 for inactive.
>  ...
>

Again, logical values.

>   To make sure I was interpreting "active"/"inactive" and the effect on what
> is RISING and FALLING edge and .bits values I wrote a short program for the
> Raspberry Pi to catch the edges and values on button press and release and
> display the results. The results were indeed that the active RISING edge was
> the transition from line-voltage to zero, with a .bits value of 1 (Hi) for
> voltage zero, and on button release the inactive FALLING edge was the
> transition from zero to line-voltage with a .bits value of 0 (Lo) for line
> voltage.
>
>   (if interested the short test program and Makefile for the Pi are at https://github.com/drankinatty/pi-wo-root/tree/master/tst/gpio_v2_button_value)
>

Sorry, I haven't ipaid much attention to your code - it is easier and clearer
to use the libgpiod command line tools (gpioset/gpioget/gpiomon/gpioinfo)
for examples.  They support all the flag combinations you care to test.

If something isn't working the way you expect then an example using those
tools would be very helpful.

>   My questions are:
>
>  1. is there any thread or document squirreled away that contains a
> discussion of how this rational was arrived at?
>

Logical values are reported, not physical, to allow users to invert the
sense of active low lines.  And given that, logical values have to be
used throughout for consistency.

If you don't want that then don't set active low, so logical IS physical.

I doubt that warranted a detailed discussion.

>  2. should the documentation at
> https://docs.kernel.org/userspace-api/gpio/chardev.html be updated to add
> the behavior for "active"/"inactive" and what flag this is dependent on
> (PULL_UP, ACTIVE_LOW or BOTH?) If so, I'd be glad to take a crack at the
> write-up and pass it to whoever for review and revision. (just let me know
> who the right person is to send it to if interested) The chardev.html seems
> like the right place for it rather than having to also locate and read
> gpio.h to find "active" and "inactive" (especially for newer users using
> latest libgpio for Pi, etc.. based on gpio_v2)
>

To update the documentation you would edit the appropriate file(s) and
submit a patch to this list, as well as those indicated in the
MAINTAINERS list (you can use linux/scripts/get_maintainers.pl on your patch
to find them).

>  3. is the expected programming approach to query the line config so that
> the .bits values can be interpreted as either 1 (Hi or Lo), or 0 (Lo or Hi)?
>

You are coming from the Pi world so there is a good chance you have a set
and forget mindset, but that is not how it is intended to be used.

The approach is that YOU are expected to know how YOU configured the
flags for the line.  You have to configure the line as part of requesting
it - even if you only want to read it, so you definitely configured it.
Why did you forget?

And if you always want physical values then never set ACTIVE_LOW.

>  (I guess that was where the biggest confusion was -- that a .bits value 0
> didn't mean no voltage, and vice-versa)
>

Again, they are logical values, not voltages.

>   Don't take this as a knock on the implementation, I think gpio_v2 is a
> stroke of genius, especially the debounce, but rather these were the parts
> that really were a bit difficult to suss out of the documentation and it may
> be helpful to include further explanation in the chardev.html pages
> explaining this a bit further.
>
> NOTE Also:
>
>   Links for the lists in
> https://docs.kernel.org/process/submitting-patches.html under the "Select
> the recipients for your patch" heading still point to
> http://vger.kernel.org/vger-lists.html (Majordomo)
>

Not sure what your point is.  The MAINTAINERS list is the definitive place
to find the appropriate list - "The best place to look for mailing lists is
in the MAINTAINERS file packaged with the kernel source."[1].

That doc is itself part of the kernel documentation, so suggest a patch to
the appropriate list - it is out of the scope of this one.


I'll have another read of the GPIO documentation with this in mind and
see where logical/physical line values can be clarified.

Cheers,
Kent.

[1] https://docs.kernel.org/process/2.Process.html#mailing-lists

Re: Documentation for line_config PULL_UP, effect on line_event edges and line_request values?

[David C. Rankin]

On 6/3/24 21:43, Kent Gibson wrote:
> Does highlighting that line values are logical help?

Thank you Kent for the reply!

   Chuckling... yes, flashing attributes too and a sledge-hammer waiting to 
fall on my head may also help.

   All kidding aside, the reason I bring it up, is if this is something that I 
scratched my head about after reading the documentation and the header, then 
I'm not alone. It's always the case with documenting code, when you are the 
author (or closely involved in creating the code), everything seems very clear 
and obvious -- you've lived with the code for months or years...

   However, on the other end of the understanding continuum, is the person 
looking to use the v2 uABI for the first time, without any familiarity with 
the code, that reads the chardev.html document will likely never appreciate 
the careful use of the words active/inactive. The comments in gpio.h header 
really do seem to just make that point more clear despite having the same 
text. I know it was that way for me.

   It may not take much of an addition at all to emphasize the logical edge 
and value relationship. In fact, just what you explained in your reply would 
make a perfect addition to help clarify and bridge the gap between those who 
know the uABI inside and out and those who just start working with it.

   Without looking at the code (or isolating the PULL and ACTIVE_LOW) it 
wasn't immediately clear which one was flipping the logical relationship. From 
a hardware standpoint it would make sense that either one could do it. Your 
explanation of bias being physical and the ACTIVE_LOW flag being the one that 
sets the logic makes that point clear. That would be a great addition to the 
doc as well.

>>
>>  * @GPIO_V2_LINE_FLAG_EDGE_RISING: line detects rising (inactive to active)
>>  * edges
>>  * @GPIO_V2_LINE_FLAG_EDGE_FALLING: line detects falling (active to
>>  * inactive) edges
>>  ...
> 
> So that does not makes it clear that the edge definitions are based on
> logical values?
> 

   That does make it clear, but for whatever human-factors reason, it is not 
as apparent in the enum gpio_v2_line_flag section of the chardev.html web 
page. Maybe it just has to do with the way the web-page puts the explanation 
on a separate line below in smaller serif font? But it almost seems the header 
screams "Pay attention to this!" while the doc just reads "Here is some other 
info too". Your idea of highlighting or at least bolding the "(inactive to 
active)" and "(active to inactive)" would certainly help there.

   In chardev.html, adding your explanation on the logical/physical difference 
would work great as a "Note: ...." right before the enum gpio_v2_line_flag 
block (or right after whichever you prefer)

   Anyway, all just good ideas intended to improve the ease of initial 
understanding for what is a great improvement over the v1 uABI. I'll leave the 
rest in your hands, you provided a great, short and concise explanation of the 
logical verses physical implementation of edge detection and value behavior. 
I'd only ask that you give serious thought to adding a few sentences or a 
paragraph precisely as you did in the reply. That really can make all the 
difference in understanding for someone coming anew to the gpio_v2 ABI.

   Thanks again for your reply.

-- 
David C. Rankin, J.D.,P.E.

Re: Documentation for line_config PULL_UP, effect on line_event edges and line_request values?

[Kent Gibson]

On Mon, Jun 03, 2024 at 11:39:16PM -0500, David C. Rankin wrote:
> On 6/3/24 21:43, Kent Gibson wrote:
> > Does highlighting that line values are logical help?
>
> Thank you Kent for the reply!
>

Btw, when replying to the list, reply-all so all included in the thread
get a copy directly rather than having to check the list - I only just
noticed this one now, some eight hours after you posted.

>   Chuckling... yes, flashing attributes too and a sledge-hammer waiting to
> fall on my head may also help.
>
>   All kidding aside, the reason I bring it up, is if this is something that
> I scratched my head about after reading the documentation and the header,
> then I'm not alone. It's always the case with documenting code, when you are
> the author (or closely involved in creating the code), everything seems very
> clear and obvious -- you've lived with the code for months or years...
>
>   However, on the other end of the understanding continuum, is the person
> looking to use the v2 uABI for the first time, without any familiarity with
> the code, that reads the chardev.html document will likely never appreciate
> the careful use of the words active/inactive. The comments in gpio.h header
> really do seem to just make that point more clear despite having the same
> text. I know it was that way for me.
>
>   It may not take much of an addition at all to emphasize the logical edge
> and value relationship. In fact, just what you explained in your reply would
> make a perfect addition to help clarify and bridge the gap between those who
> know the uABI inside and out and those who just start working with it.
>
>   Without looking at the code (or isolating the PULL and ACTIVE_LOW) it
> wasn't immediately clear which one was flipping the logical relationship.
> From a hardware standpoint it would make sense that either one could do it.
> Your explanation of bias being physical and the ACTIVE_LOW flag being the
> one that sets the logic makes that point clear. That would be a great
> addition to the doc as well.
>

Agreed - I'm looking into adding some clarification to the docs.

> > >
> > >  * @GPIO_V2_LINE_FLAG_EDGE_RISING: line detects rising (inactive to active)
> > >  * edges
> > >  * @GPIO_V2_LINE_FLAG_EDGE_FALLING: line detects falling (active to
> > >  * inactive) edges
> > >  ...
> >
> > So that does not makes it clear that the edge definitions are based on
> > logical values?
> >
>
>   That does make it clear, but for whatever human-factors reason, it is not
> as apparent in the enum gpio_v2_line_flag section of the chardev.html web
> page. Maybe it just has to do with the way the web-page puts the explanation
> on a separate line below in smaller serif font? But it almost seems the
> header screams "Pay attention to this!" while the doc just reads "Here is
> some other info too". Your idea of highlighting or at least bolding the
> "(inactive to active)" and "(active to inactive)" would certainly help
> there.
>
>   In chardev.html, adding your explanation on the logical/physical
> difference would work great as a "Note: ...." right before the enum
> gpio_v2_line_flag block (or right after whichever you prefer)
>

I'm not sure that is possible, given the way the documentation is generated
from reST.

And, given the way the html documentation is structured, my preference would
be to expand the documentation of the relevant ioctls and reads, as that is
where a reader trying to understand what the function is doing would be
looking.

>   Anyway, all just good ideas intended to improve the ease of initial
> understanding for what is a great improvement over the v1 uABI. I'll leave
> the rest in your hands, you provided a great, short and concise explanation
> of the logical verses physical implementation of edge detection and value
> behavior. I'd only ask that you give serious thought to adding a few
> sentences or a paragraph precisely as you did in the reply. That really can
> make all the difference in understanding for someone coming anew to the
> gpio_v2 ABI.
>
>   Thanks again for your reply.

Thanks for the feedback.

Oh, and are you ok with me adding you as suggesting the patch?

Cheers,
Kent.

Re: Documentation for line_config PULL_UP, effect on line_event edges and line_request values?

[David C. Rankin]

On 6/4/24 07:52, Kent Gibson wrote:
> Thanks for the feedback.
> 
> Oh, and are you ok with me adding you as suggesting the patch?
> 

That is well above my pay-grade in the kernel world. A patch sounds great, but 
I'll leave it in your capable hands how to do it. I'm just thankful that the 
little extra addition to the documentation can make all the difference in 
helping the "I got it!" light-bulb wink on for new users coming to the interface.

I know it will help given the threads on the Pi forum I've participated in. 
With the added benefit of helping the 3rd party libraries move from the V1 ABI 
to V2 more easily (e.g. wiringpi, etc..)

Let me know if you need anything else, otherwise, thank you again for your 
reply and help here.

-- 
David C. Rankin, J.D.,P.E.