Skip to content

Names "is_high", "is_low" is confusing. #71

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

Closed
janderholm opened this issue Apr 7, 2018 · 10 comments
Closed

Names "is_high", "is_low" is confusing. #71

janderholm opened this issue Apr 7, 2018 · 10 comments

Comments

@janderholm
Copy link

The function names for at InputPin and OutputPin (is_high, is_low) can be a bit confusing for MCUs with inverting logic. At least some (I would imagine most?) NXP LPC microcontrollers have a register for inverting the value of the GPIO register. If the invert bit is set, a high voltage at the pin will result in a 0 in the register. The name is_low suggests the voltage is low.

I don't know if there's a better name though. is_active?

See for example page 76 in the User Manual for the LPC81x series

@janderholm janderholm changed the title Names is_high, is_low is confusing. Names "is_high", "is_low" is confusing. Apr 7, 2018
@thejpster
Copy link
Contributor

is_set(), is_clear(), set(), clear()?

@adamgreig
Copy link
Member

set/clear are nice verbs for output pins but it seems a bit unusual to talk about is_set on an input pin. I think it's fair to just say if the input pin is_high then it's a logic high, and if the specific MCU has inverted the sense of the pins, that means logic highs correspond to low voltages.

In other words high/low refer to logic states of the pins and usually that refers to voltage levels, but not on inverted pins.

@therealprof
Copy link
Contributor

@Fulkerson It's specifically the job of a hal implementation to ensure that is_high() yields true if the logic level on a pin is high. If there's some magic that may change the interpretation of a register read then it's the job of the implementation to translate it correctly into a high/low level.

@janderholm
Copy link
Author

@therealprof is_low and is_high can certainly be useful if you actually need to figure out if the pin is grounded or not but that is usually not the case. Normally you just set the invert register to make the logic levels of the port registers make sense depending what is connected.

Lets say you have something shifting in bytes on a port. Some of the pins, for whatever reason, is inverted. Instead of adding instructions to flip some of the bits you just set the inverting register and read the bytes from the port directly.

@therealprof
Copy link
Contributor

@Fulkerson

but that is usually not the case

I could not disagree more. That is exactly what you want. The HAL is meant to abstract the hardware so that there is a common interface translating between generic drivers and hardware. If a driver needs to know whether the voltage level on a pin is high or low it would ask for that information using the is_high() or is_low() method and it's the HAL implementations responsibility to reliably deliver the correct result and abstract over the fact that somehow the register is inverted.

Lets say you have something shifting in bytes on a port. Some of the pins, for whatever reason, is inverted. Instead of adding instructions to flip some of the bits you just set the inverting register and read the bytes from the port directly.

As I said: if the MCU knows that the result is inverted it's the implementations job to abstract it. If this is hardware choice (i.e. GPIO pin is inverted via external hardware) then it's the job of the application to make sure the inversion is properly accounted for.

If you really wanted to support that inversion in an hal implementation for the LPC810 (and others) you can also implement a separate InputPin/OutputPin state which sets this up and accounts for it in the implementation of is_low() and is_high().

@janderholm
Copy link
Author

@adamgreig Fair enough. I usually work higher up in the stack and not a lot with microcontrollers. On Linux embedded systems it is common to specify whether something is active high or active low and configure the port so that when the peripheral (be it a button or amplifier or whatever) is considered active (button is pressed, amplifier is powered on etc) it corresponds to a digital one.

In the long run I think it is important that the HAL clearly states what the methods are. I.e. is low the logic or the actual state of the physical pin? Otherwise it's easy to end up with different implementations behaving differently.

@therealprof
Copy link
Contributor

On Linux embedded systems it is common to specify whether something is active high or active low and configure the port so that when the peripheral (be it a button or amplifier or whatever) is considered active (button is pressed, amplifier is powered on etc) it corresponds to a digital one.

We're doing that, too. ;) But the interpretation of what is active or inactive is left to a specific driver or the application and not handled by the lower layers.

In the long run I think it is important that the HAL clearly states what the methods are. I.e. is low the logic or the actual state of the physical pin? Otherwise it's easy to end up with different implementations behaving differently.

Yes, that situation is actually a bit more complex than it may sound on first glance. There's plenty of discussion about the details in #29 and at least another one I can't find right now.

@janderholm
Copy link
Author

I could not disagree more. That is exactly what you want. The HAL is meant to abstract the hardware so that there is a common interface translating between generic drivers and hardware. If a driver needs to know whether the voltage level on a pin is high or low it would ask for that information using the is_high() or is_low() method and it's the HAL implementations responsibility to reliably deliver the correct result and abstract over the fact that somehow the register is inverted.

I have a hard time thinking of a driver that needs to know what the actual voltage on a digital pin is. Usually a driver only care about the peripheral on the other end of the line and not what components are in the way. There are many times the electrical signal may be inverted on the way to the destination. Many times the protocol dictates how the electrical connection should look like, but it is far from always the case.

If you really wanted to support that inversion in an hal implementation for the LPC810 (and others) you can also implement a separate InputPin/OutputPin state which sets this up and accounts for it in the implementation of is_low() and is_high()

That is sort of my point! I envision myself adding a mode for InputPin that is Inverted or something along the line which sets the INV register appropriately but at that point I think the high/low wording becomes confusing. I am used to refer to high/low when discussing specifics of a schematic. There voltage is the only thing that matters. When translating that to code the constant inversions becomes overly complicated for my poor brain. Especially when working with lots of pins in a large code base. I would rather set up my gpios once and not having to refer to the schematics 6 months later.

We're doing that, too. ;) But the interpretation of what is active or inactive is left to a specific driver or the application and not handled by the lower layers.

That is perfectly fine for simple things like LEDs (is it active low or high?) but driver code quickly becomes very complex when adding interrupt pins to an ethernet driver or whatever, and you have to specify for each raw GPIO the driver is using if it is active high or not. Better put it in the gpio interface like with pullups and similar in my opinion.

I believe we are of the same opinion the matter. Whether a pin should invert its logic or not should be a configuration of the pin. If it is configured to be non inverting, low should be grounded. If it is configured to be inverting high should be grounded. My main argument is that I think using high and low is confusing when introducing inversion to pins.

@therealprof
Copy link
Contributor

I have a hard time thinking of a driver that needs to know what the actual voltage on a digital pin is.

The real voltage is irrelevant. The only question is: Is it considered a high level or a low level (or is it floating/in between). There're very clear definitions for that.

Usually a driver only care about the peripheral on the other end of the line and not what components are in the way.

Exactly.

I am used to refer to high/low when discussing specifics of a schematic.

And this goes hand in hand with what the HAL does.

I would rather set up my gpios once and not having to refer to the schematics 6 months later.

Not sure what's preventing you from doing that. That's exactly how it's supposed to work: The application sets up the peripherals using the hal implementation according to the schematics and then uses them (either directly or by handing them off to some driver).

but driver code quickly becomes very complex when adding interrupt pins to an ethernet driver or whatever

How so? The whole point of the abstraction is to keep the drivers simple and generic.
A driver doesn't have to (and shouldn't!) worry about interrupts at all, that's the job of an interrupt handler which has to be platform specific anyway. But yeah, interrupts are still kind of a sad story, the setup is far too manual...

My main argument is that I think using high and low is confusing when introducing inversion to pins.

And I still don't get why. Can you link any datasheet of any active component that does not mention high or low? Any digital signal consists of high/low levels/edges so it is important to know what the current state directly at the pin is if you want to use it to work with an external component. If you have an external inversion somewhere then you'll obviously need to account for that fact in your setup but it doesn't change anything about the level directly at the pin. An is_high() in a regular pin configuration corresponds to a logic high level you could measure with a multimeter on that very pin.

Most likely I would even argue against supporting that inversion flag on the GPIO. I can see why it exists for other languages where software signal inversion might cost instruction cycles, but in Rust any level-inverting adapter on a regular pin (which will work with any MCU) shouldn't require any additional instructions.

@janderholm
Copy link
Author

I think I see what you mean. I can implement the InputPin trait on top of an Input and invert it there. I did not think about that!

peckpeck pushed a commit to peckpeck/embedded-hal that referenced this issue Nov 10, 2022
71: WIP: Update embedded-hal to version 1.0.0-alpha.6 r=eldruin a=caemor

The delay update is pretty straightforward. 

The error update is more difficult: The current implementation creates a new struct including a single element: the upstream linux error, and implement the e-h error trait for it. I've got two issues with this.
Is creating a new struct the best way to do this?
For matching the e-h trait kind: Currently I just matched everything to `Other` because I couldn't find any good matches when taking a quick glance.  Is there something that should be matched?
The creation of a central error enum for linux e-h would be an other alternative way to solve this.


Co-authored-by: Caemor <[email protected]>
Co-authored-by: Chris <[email protected]>
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

4 participants