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

stm32: Add support for analog-only "_C" pins. #13764

Merged

Conversation

dpgeorge
Copy link
Member

@dpgeorge dpgeorge commented Feb 27, 2024

This PR adds support for the analog-only pads/pins on STM32H7 parts. These pads/pins are called PA0_C/PA1_C/PC2_C/PC3_C in the datasheet. They each have an analog switch that can optionally connect them to their normal pin (eg PA0). When the switch is open, the normal and _C pin are independent pins/pads.

The approach taken in this PR to make these _C pins available to Python is:

  • put them in their own, independent row in the stm32h7_af.csv definition file, with only the ADC column defined (this seems logical to me, since they are going to be separate machine.Pin entities, and doing it this way keeps make-pins.py pretty clean)
  • allow a board to reference these pins in the board's pins.csv file by the name PA0_C etc (so a board can alias them, for example)
  • these pins (when enabled in pins.csv) now become available like any other machine.Pin through both machine.Pin.board and machine.Pin.cpu
  • BUT these _C pins have a separate pin type which doesn't have any methods, because they don't have any functionality
  • these _C pins can be used with machine.ADC to construct the appropriate ADC object, either by passing the string as machine.ADC("PA0_C") or by passing the object as machine.ADC(machine.Pin.cpu.PA0_C)
  • if a board defines both the normal and _C pin (eg both PA0 and PA0_C) in pins.csv then it must not define the analog switch to be closed (this is a sanity check for the build, because it doesn't make sense to close the switch and have two separate pins)

To show how this works, the ARDUINO_PORTENTA_H7 board is updated to define its analog pins, using these new _C pins. The code is also tested on this board to work correctly.

TODO:

  • update all relevant af.csv files with separate _C rows

@dpgeorge
Copy link
Member Author

@iabdalkader FYI, this should allow full support for the dual-pad analog pins on Arduino boards.

@@ -59,6 +59,11 @@ class Stm32Pin(boardgen.Pin):
def __init__(self, cpu_pin_name):
super().__init__(cpu_pin_name)

# Pins ending in "_C" are special analog-only pins.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add "H7" and "Dual-pad" to this comment -- if you don't know about dual pad, it would be very difficult to figure out what an "analog-only pin" is.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

.adc_num = p_adc_num, \
.adc_channel = p_adc_channel, \
}
#endif
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we add an #else here that did something like

#define PIN_ANALOG DUAL_PAD_SUPPORT_NOT_ENABLED__CONFIGURE__MICROPY_HW_ANALOG_SWITCH_PAx

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

// analog switch to be open. The macro code below will produce a compiler warning
// if the board defines the switch as closed.
#if defined(pin_A0) && defined(pin_A0_C)
#define MICROPY_HW_ANALOG_SWITCH_PA0 (SYSCFG_SWITCH_PA0_OPEN)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the idea that this triggers a redefinition of the macro?

Would doing this achieve the same:

#if defined(pin_A0) && defined(pin_A0_C) && defined(MICROPY_HW_ANALOG_SWITCH_PA0) && MICROPY_HW_ANALOG_SWITCH_PA0 == SYSCFG_SWITCH_PA0_CLOSED
#error "Both pads for dual-pad A0 are enabled, switch must be closed."
#endif

(It's a bit confusing to see a .c file defining a macro that it doesn't use)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the comment says, this is intended to trigger a compiler warning if the board defines the switch as closed.

I originally had tried what you suggested above but it doesn't work because the comparison MICROPY_HW_ANALOG_SWITCH_PA0 == SYSCFG_SWITCH_PA0_CLOSED is too complex for the preprocessor.

@iabdalkader
Copy link
Contributor

@dpgeorge I tested this by adding all analog/analog-only pins to the Giga board, and I can read values independently on dual pads pins, so it seems to be working fine, thanks!

A0,PC4
A1,PC5
A2,PB0
A3,PB1
A4,PC3
A5,PC2
A6,PC0
A7,PA0
A8,PC2_C
A9,PC3_C
A10,PA1_C
A11,PA0_C

Test:

import time
from machine import ADC
from machine import Pin

adc1 = ADC("A5")  # PC2
adc2 = ADC("A8")  # PC2_C
while True:
    print(adc1.read_u16(), adc2.read_u16())
    time.sleep_ms(500)

@dpgeorge dpgeorge force-pushed the stm32-adc-analog-only-pin-support branch from 750eadd to c727b3f Compare March 1, 2024 05:24
@dpgeorge
Copy link
Member Author

dpgeorge commented Mar 1, 2024

Thanks @iabdalkader for testing.

I have now added the Giga pins.csv code as part of this PR.

Does anything need to be added for Nicla Vision?

@iabdalkader
Copy link
Contributor

The Nicla closes 2 switches, and breaks out 3 single pad pins analog pins::

A0,PC4
A1,PF13
A2,PF3

Pins for all boards can be found here in <board>/variant.cpp https://github.com/arduino/ArduinoCore-mbed/tree/main/variants

Should we also add digital board pins in this PR ?

@dpgeorge
Copy link
Member Author

dpgeorge commented Mar 4, 2024

The Nicla closes 2 switches, and breaks out 3 single pad pins analog pins

OK, thanks, I added those analog pins.

Should we also add digital board pins in this PR ?

I also added digital pins to this PR. Can you please check them? I didn't add everything for the Giga, just the standard set up to D14.

Comment on lines 175 to 176
D4,PC7
D5,PC6
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
D4,PC7
D5,PC6

Note those two pins are broken out on the camera connector. I don't think it makes much sense to define them, since the camera connector isn't a generic B2B connector, but if we do then we should define the other pins on that connector.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These pins have not been added.

@iabdalkader
Copy link
Contributor

Can you please check them? I didn't add everything for the Giga, just the standard set up to D14.

I double checked all pins, they all look good. No need to add the HD pins there's another open PR that adds them

@dpgeorge
Copy link
Member Author

dpgeorge commented Mar 4, 2024

Hmm, maybe I should merge #14022 first, then this PR here just adds the _C analog pins.

@iabdalkader
Copy link
Contributor

Hmm, maybe I should merge #14022 first, then this PR here just adds the _C analog pins.

Either way is fine with me, but re #14022, in the stm32 port, it's conventional to define actual board pins near the end of the file, after all Pxy,Pxy

@sebromero
Copy link
Contributor

sebromero commented Mar 5, 2024

I just added the suggestions from the review in #14022

@projectgus
Copy link
Contributor

This is an automated heads-up that we've just merged a Pull Request
that removes the STATIC macro from MicroPython's C API.

See #13763

A search suggests this PR might apply the STATIC macro to some C code. If it
does, then next time you rebase the PR (or merge from master) then you should
please replace all the STATIC keywords with static.

Although this is an automated message, feel free to @-reply to me directly if
you have any questions about this.

@dpgeorge dpgeorge force-pushed the stm32-adc-analog-only-pin-support branch from eb0b79b to 27da007 Compare March 8, 2024 01:37
This commit adds support for the dual-analog-pads on STM32H7 parts.  These
pads/pins are called PA0_C/PA1_C/PC2_C/PC3_C in the datasheet.  They each
have an analog switch that can optionally connect them to their normal pin
(eg PA0).  When the switch is open, the normal and _C pin are independent
pins/pads.

The approach taken in this commit to make these _C pins available to Python
is:
- put them in their own, independent row in the stm32h7_af.csv definition
  file, with only the ADC column defined (they are separate machine.Pin
  entities, and doing it this way keeps make-pins.py pretty clean)
- allow a board to reference these pins in the board's pins.csv file by the
  name PA0_C etc (so a board can alias them, for example)
- these pins (when enabled in pins.csv) now become available like any other
  machine.Pin through both machine.Pin.board and machine.Pin.cpu
- BUT these _C pins have a separate pin type which doesn't have any
  methods, because they don't have any functionality
- these _C pins can be used with machine.ADC to construct the appropriate
  ADC object, either by passing the string as machine.ADC("PA0_C") or by
  passing the object as machine.ADC(machine.Pin.cpu.PA0_C)
- if a board defines both the normal and _C pin (eg both PA0 and PA0_C) in
  pins.csv then it must not define the analog switch to be closed (this is
  a sanity check for the build, because it doesn't make sense to close the
  switch and have two separate pins)

Signed-off-by: Damien George <damien@micropython.org>
@dpgeorge dpgeorge force-pushed the stm32-adc-analog-only-pin-support branch from 27da007 to a9efffc Compare March 8, 2024 01:38
@dpgeorge
Copy link
Member Author

dpgeorge commented Mar 8, 2024

I have removed all pin additions to Arduino boards. That will be done as part of #14022.

@dpgeorge dpgeorge merged commit a9efffc into micropython:master Mar 8, 2024
7 checks passed
@dpgeorge dpgeorge deleted the stm32-adc-analog-only-pin-support branch March 8, 2024 01:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants