Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ambiguity of Line.request()'s default arg polarity in light of ACTIVE_LOW outputs #78

Open
JonathonReinhart opened this issue Dec 18, 2023 · 5 comments

Comments

@JonathonReinhart
Copy link

Background

Line.request() accepts LineRequestFlags including ACTIVE_LOW. This inverts the polarity of the value in the LineHandle::get_value() and LineHandle::set_value() APIs to mean:

  • 0 -> inactive -> electrically high
  • 1 -> active -> electrically low

Problem

Line.request() also accepts a default argument which is currently documented as:

For an output, the default parameter specifies the value the line should have when it is configured as an output.

The polarity of this argument is not clear.

If I do this:

line.request(LineRequestFlags.ACTIVE_LOW, 0, "me");

does 0 mean:

  • inactive -> electrically high
  • electrically low

Somewhat related: #49

@JonathonReinhart
Copy link
Author

Initially, I would expect the polarity of default to match that of set_value(value): its polarity should respect ACTIVE_LOW.


cpio-cdev doesn't do anything with default other than stick it in the default_values array of the gpiohandle_request.

In the Linux uAPI, default_values is documented as:

* @default_values: if the %GPIOHANDLE_REQUEST_OUTPUT is set for a requested
* line, this specifies the default output value, should be 0 (low) or
* 1 (high), anything else than 0 or 1 will be interpreted as 1 (high)

This is still not abundantly clear, and by saying "0 (low)", implies the opposite of what I would have expected: That this value is unaffected by ACTIVE_LOW.

@JonathonReinhart
Copy link
Author

JonathonReinhart commented Dec 18, 2023

Kernel Analysis

linehandle_create() -- the function which handles the GPIO_GET_LINEHANDLE_IOCTL invoked by request() -- calls gpiod_direction_output() to put the GPIO in output mode. This includes the !! boolean-ified default_value:

		if (lflags & GPIOHANDLE_REQUEST_OUTPUT) {
			int val = !!handlereq.default_values[i];

			ret = gpiod_direction_output(desc, val);

gpiod_direction_output() will then invert value if necessary (according to FLAG_ACTIVE_LOW) before passing it to gpiod_direction_output_raw_commit():

/**
 * gpiod_direction_output - set the GPIO direction to output
 * @desc:	GPIO to set to output
 * @value:	initial output value of the GPIO
 *
 * Set the direction of the passed GPIO to output, such as gpiod_set_value() can
 * be called safely on it. The initial value of the output must be specified
 * as the logical value of the GPIO, i.e. taking its ACTIVE_LOW status into
 * account.
 *
 * Return 0 in case of success, else an error code.
 */
int gpiod_direction_output(struct gpio_desc *desc, int value)
{
	int ret;

	VALIDATE_DESC(desc);
	if (test_bit(FLAG_ACTIVE_LOW, &desc->flags))
		value = !value;
	else
		value = !!value;

	// ...

	return gpiod_direction_output_raw_commit(desc, value);

Conclusion: default_values do respect GPIOHANDLE_REQUEST_ACTIVE_LOW.

@JonathonReinhart
Copy link
Author

Empirical Analysis

I have a board with an active-low output that drives a reset line.

Example code 1 (good)

const COMMON_DELAY: Duration = Duration::from_millis(1);

reset_hand = line.request(
    gpio_cdev::LineRequestFlags::OUTPUT | gpio_cdev::LineRequestFlags::ACTIVE_LOW,
    0, // Inactive (high)
    GPIO_CONSUMER
)

thread::sleep(Duration::from_millis(3000));  // For testing

// Put chip in reset
reset_hand.set_value(1)?
thread::sleep(COMMON_DELAY);

// ... other stuff

// Bring chip out of reset
reset_hand.set_value(1)?

With that code, I observe no glitches. I see the line start high, stay there for the 3 second pause, go low for the expected amount of time (1-2 ms), then return high.

That tells me that there were no unexpected transitions due to an incorrect initial value.

Example code 2 (bad)

If I make the following change:

 reset_hand = line.request(
     gpio_cdev::LineRequestFlags::OUTPUT | gpio_cdev::LineRequestFlags::ACTIVE_LOW,
-    0, // Inactive (high)
+    1, // WRONG: Incorrectly assumes this means electrically high
     GPIO_CONSUMER
 )

Then I see my reset line drop immediately, and stay low for the entire 3 second pause, before returning high.

This tells me that the 1 initial value meant "active" and drove my line low.


Conclusion: default does respect ACTIVE_LOW.

@JonathonReinhart
Copy link
Author

Summary:

  • Both the kernel source analysis and empirical analysis come to the same conclusion: default does respect ACTIVE_LOW.
  • The kernel uAPI docs could be improved in this regard.
  • The gpio-cdev docs could be improved in this regard.

@warthog618
Copy link

warthog618 commented Feb 11, 2024

* Both the [kernel source analysis](https://github.com/rust-embedded/gpio-cdev/issues/78#issuecomment-1861562611) and [empirical analysis](https://github.com/rust-embedded/gpio-cdev/issues/78#issuecomment-1861642126) come to the same conclusion: `default` _does_ respect `ACTIVE_LOW`.

* The kernel uAPI docs could be improved in this regard.

Thanks for the feedback. Just curious - if the kernel did NOT honor the active low flag then what purpose would the flag serve? I've always thought that the very existence of that flag implies that line values are logical, not physical.

Perhaps the documentation could be clearer, specifically that line values are logical not physical and that the active low flag controls the mapping between the two, but uAPI v1 is deprecated so it doesn't get much attention.

Hopefully the uAPI v2 documentation is a bit clearer - it uses active and inactive for the logical states and avoids mention of high and low (the physical states):

 * @GPIO_V2_LINE_FLAG_ACTIVE_LOW: line active state is physical low

...

 * 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

Similarly, my gpiocdev library also uses the active/inactive terminology for line values, and documents the logical/physical mapping.

I'll see about a patch for the uAPI v1 docs to replace low/high with inactive/active.

Update: documentation patches submitted to the kernel and on track to make it into 6.9.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants