search

AdaCore / Ada_Drivers_Library

Ada source code and complete sample GNAT projects for selected bare-board platforms supported by GNAT.
BSD 3-Clause "New" or "Revised" License
248 stars 144 forks source link

STM32: Add preconditions on setting/driving GPIO pins actually be configured as an output #338

Closed WickedShell closed 4 years ago

WickedShell commented 4 years ago

This adds preconditions that before trying to drive a GPIO pin as an output that it must actually be configured as an output. I had a problem where a pin was not getting configured correctly that the existing post condition on Drive was catching, but having caught it as a precondition that the port was actually an output would have been far more useful from a debug time as the post condition sent me on a couple of long rabbit holes before I realized the pin was in the wrong mode.

I'm not fully sure there is not a valid reason to call these without having set it as an output however. I can't seem to find one though. As an example at the moment any attempt to call Drive will fail if the pin isn't externally driven to the target state.

This works well with my admittedly simple test cases here, but if I've missed reasons why this doesn't work I'm happy to tweak it further (or just drop it if needed). Finally before the CLA bot complains the signed CLA was e-mailed in previously.

CLAassistant commented 4 years ago

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

pat-rogers commented 4 years ago

Postconditions on these routines are problematic because the underlying connected hardware may change the state of the pin in response to the requested change, prior to the postcondition execution. For example, we Set a pin high and the hardware then clears that pin as part of its response. Then the postcondition finally gets around to being executed and fails. I don't recall the specific hardware case in which that happened when developing the STM32 drivers, but it happened.

WickedShell commented 4 years ago

@pat-rogers I absolutely agree and had the same thought. I briefly played around with the Robotics_with_Ada quadrature encoder example and the post condition on Reset_Count actually would randomly fail for exactly this reason if the encoder was still moving.

The post conditions on set work by reading the input back, which only happens after an AHB clock cycle for example. With that being said the post condition on Drive was already here (and actually saved me time by at least killing me on this line rather then leaving me to probe the hardware a bunch to figure it out :D).

I'm happy to drop the post conditions, particularly if we add the preconditions, since at that point the post condition should be dependent upon hardware configuration, and as you said avoids some of the edge cases where it blows up. At a minimum though I think the checks on Set/Clear should be the same as those on Drive.

Fabien-Chouteau commented 4 years ago

Some hardware support the GPIO being both input and output at the same time. In that case you may want to set and point that is configured as input. So you cannot have a precondition on the mode for Set().

Or you would have to add an Input_Output mode that says both modes at the same time.

WickedShell commented 4 years ago

@Fabien-Chouteau Is there any STM32 hardware the behaves that way? I know you can read back the current pin state while it's an output (and the added precondition doesn't stop that), but I'm unaware of any hardware that allows you to set a pin while it's configured as an input for example. (At least I can't find it in the manual, and from inadvertently testing it on F4's it doesn't seem to do anything).

Fabien-Chouteau commented 4 years ago

Is there any STM32 hardware the behaves that way?

HAL.GPIO is not only for STM32s. I think the nRF51 does support that.

Matthias-R commented 4 years ago

And sometimes, you have to set the output register before changing the direction register to avoid glitches...

WickedShell commented 4 years ago

HAL.GPIO is not only for STM32s. I think the nRF51 does support that.

I was putting this in the STM32 one so it wouldn't have affected the nRF support.

And sometimes, you have to set the output register before changing the direction register to avoid glitches...

That's definitely a valid point, however.

Based on the above comments then I guess this should be closed as is, but the postcondition on drive should be removed? It stands out that this one has it, and none of the others do.

pat-rogers commented 4 years ago

Based on the above comments then I guess this should be closed as is, but the postcondition on drive should be removed? It stands out that this one has it, and none of the others do.

I agree, that one should go.