gpio subsystem, gpio chips and improvements

It's all about the code!
Post Reply
User avatar
Dron_Gus
contributor
contributor
Posts: 450
Joined: Wed Nov 13, 2013 1:11 pm
Location: S-Pb
Github Username: dron0gus

gpio subsystem, gpio chips and improvements

Post by Dron_Gus »

Hi all.
Due to different timezones I can not participate in online discussions in Slack. So I'm creating this thread.
Andrey asked me to add push-pull support in TLE8888 driver. During this task I found few architecture issues that already cause some problems.

1. lot of code uses STM32/ChibiOS pin config type iomode_t (uint32_t). Like PAL_MODE_INPUT_PULLUP that is actually (PAL_STM32_MODE_INPUT | PAL_STM32_PUPDR_PULLUP). These defines are good for direct write to STM32 gpio registers. But this is pain when we work with external gpio chip.
We have pin_input_mode_e. But it is converted to iomode_t by getInputMode before efiSetPadMode call.
Also we have pin_output_mode_e. But most of code just use OM_DEFAULT.
So efiSetPadMode is called with STM32-specific iomode_t mode as argument. If pin is not native STM32 gpio, then we had to either pass this 'mode' as-is to driver of convert it back to some generic enum.

2. There is a mess with logic/physical level.
When we call setValue(true) for STM32 native pin, STM32 will output logic '1'.
When we call setValue(true) for any of currently supported gpio chip, this chip will enable its output FET and output will be 0V.
This works fine while we have MOSFETs connected to STM32 native pins. These MOSFETs 'invert' signals.
So everything looks fine: setValue(true) will enable load and setValue(false) will disable.
But now we have TLE8888 Push-Pull signals.
Should driver enable low switch (and output 0V) when setValue(true) is called? Should it enable high switch and output 12V when setValue(false)? This is a bit confusing.
Also we should be able to disable both low and high switches to set output to Hi-Z. This is need for freewheeling of DC motor or disabling of coil of stepper motor.
Should this be done by something like setValue(-1) call? Or we should do through efiSetPadMode?

3. a lot of configurable signals have two entries in config: output_pin_e + pin_output_mode_e or brain_input_pin_e + pin_input_mode_e where:
typedef brain_pin_e output_pin_e;
So pin is defined as two 8bit values.
May it will be more convenient to use single 16 bit field for defining pin and its settings? What is the reason to have output_pin_e and brain_input_pin_e?

4. BTW, brain_pin_e is getting to its limit (currently it is 8 bit). May be switch to 16bits for pin number?
Then 32 bit value can be used to store pin number and its settings: input/output, AF, pull-up/down, OD/PP and so on. 16 bits for pin number, 16 bit for settings.

Not sure how much efforts these modifications can take, but it can help us avoid problems in future.
User avatar
AndreyB
Site Admin
Posts: 14292
Joined: Wed Aug 28, 2013 1:28 am
Location: Jersey City
Github Username: rusefillc
Slack: Andrey B

Re: gpio subsystem, gpio chips and improvements

Post by AndreyB »

1. yes, we probably have an issue here - gpio chips are a relatively new thing and we have not yet identified a clean universal solution. that's a TODO here for sure.

2. not exactly. first of all, stm32 pin usually also connects to a MOSFET and often setValue(true) means +3v on stm32 but low-side control on the MOSFET. But what's really is important is that setValue(true) means "turn fuel pump ON" regardless of what it means on physical level. some fuel pumps would be low-side driven relay maybe some fuel pumps would be high-side high-current switch, but in both case "fuel pump is ON" is the same "setValue(true)"

3. for UI it's easier to deal with two fields. For example, PA13+pull-down, PA13+floating. We can probably use some struct magic to make this a bit nicer and have a composite type for pin+mode instead of currently managing those manually.

4. yes 8 bits for pin is pretty limited and 16 bits for pin line makes sense. That's a pretty simple change (very incompatible for configuration but TS would help us migrate), this one is probably the most actionable issue on this list.
Very limited telepathic abilities - please post logs & tunes where appropriate - http://rusefi.com/s/questions

Always looking for C/C++/Java/PHP developers! Please help us see https://rusefi.com/s/howtocontribute
User avatar
Dron_Gus
contributor
contributor
Posts: 450
Joined: Wed Nov 13, 2013 1:11 pm
Location: S-Pb
Github Username: dron0gus

Re: gpio subsystem, gpio chips and improvements

Post by Dron_Gus »

AndreyB wrote:
Sat Feb 08, 2020 11:50 pm
2. not exactly. first of all, stm32 pin usually also connects to a MOSFET and often setValue(true) means +3v on stm32 but low-side control on the MOSFET. But what's really is important is that setValue(true) means "turn fuel pump ON" regardless of what it means on physical level. some fuel pumps would be low-side driven relay maybe some fuel pumps would be high-side high-current switch, but in both case "fuel pump is ON" is the same "setValue(true)"
Ok. At least we need to think about value used to set output to Hi-Z for PP outputs.
AndreyB wrote:
Sat Feb 08, 2020 11:50 pm
3. for UI it's easier to deal with two fields. For example, PA13+pull-down, PA13+floating. We can probably use some struct magic to make this a bit nicer and have a composite type for pin+mode instead of currently managing those manually.
Of course it can be struct {uint16_t pin; uint16_t mode};
AndreyB wrote:
Sat Feb 08, 2020 11:50 pm
4. yes 8 bits for pin is pretty limited and 16 bits for pin line makes sense. That's a pretty simple change (very incompatible for configuration but TS would help us migrate), this one is probably the most actionable issue on this list.
Do you see any disadvantages in dividing 16 bit pin number into 8 + 8 bits? 8 bits for chip and 8 bits for pin.
Of course it can be interpreted as single 16 bit field but with gapes between chips. Is this ok for TunerStudio enums names to have gapes?
For STM32 native pins first 256 values should be enough I think.
Post Reply