Skip to content

stm32: Add analog switches configuration. #12706

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

Merged
merged 1 commit into from
Nov 3, 2023

Conversation

iabdalkader
Copy link
Contributor

@iabdalkader iabdalkader commented Oct 16, 2023

This PR adds support for dual-pad pins, allowing access to the extra ADC channels on the second pads (when the analog switch is open). Additionally the second commit adds new config options to configure the analog switches, this is not strictly necessary I can move them back to board_init.c files, but figured that eventually more boards will need to configure them.

@dpgeorge
Copy link
Member

dpgeorge commented Nov 1, 2023

For reference, here is the logic diagram of the _C pins:

stm32_c_ports

@dpgeorge
Copy link
Member

dpgeorge commented Nov 1, 2023

And the alternate function table:

stm32_c_ports_af

The columns on the left of the table are the package variants that the pads are available on. Note that both PC2 and PC2_C are only available on TFBGA240+25 packages. On other packages only PC2_C is available (same for PC3).

@dpgeorge
Copy link
Member

dpgeorge commented Nov 1, 2023

@iabdalkader which packages do the Arduino boards use?

@jimmo
Copy link
Member

jimmo commented Nov 1, 2023

This is complicated! I need to think about it some more but here are some initial thoughts.

The change in the first commit promotes "pads" to "pins", which I'm not sure makes sense. For example, on a part with both PA0 and PA0_C, I don't think you should be able to do digital IO on both.

Would someone ever want to change the switch state at runtime. i.e. a generic board that has both PC2 and PC2_C wired to external headers, there are several combinations available to them.

On a part with the pad Pxy_C (but without the pad Pxy), what should we call the pin in MicroPython. (For example, PC2_C on the the UFBGA169 package of the H747, e.g. the Nicla Vision).
a) We definitely shouldn't provide both (which I think this PR does)
b) It feels to me like it should be PC2, not PC2_C.
What if the switch is open in that case, do they really have the "pin" Pxy at all? (Really they just have an extra ADC channel wired directly to a pad, and no pin at all).

So the switches would default to open (except on parts with a Pxy_C pad but no Pxy pad, where they default to closed). (This is actually the case on the Nicla Vision -- as I understand it, the config added in this PR actually sets it to the reset default anyway -- it has only PA0, PA1, PC2_C, PC2_C, so the switches will be 0, 0, 1, 1 by default). (Is that true of all of the configuration for both boards... it's always setting the default value?)

On the Nicla, would a user ever need/want to open the switch, thereby disconnecting ULPI NXT and DIR pins?

My feeling is that we really need to address this by making the ADC more sophisticated (we already want to do this anyway via ADCBlock -- e.g. you cannot currently use more than the lowest-numbered ADC on the channel that has the most ADCs).

So for example:
If the part has only one of the dual pads (e.g. Pxy_C but not Pxy, or Pxy but not Pxy_C). The switch will default to closed in the first case, open in the second. In the af.csv we configure just Pxy, with just the ADC units and channels available to Pxy. However, we make the "direct" ADC channels available via ADCBlock (i.e. ADCBlock(3).channel(1) would get PC3_C's "direct" ADC on the H747). When you use these channels, it toggles the switch for you (in the first case, to isolate from the pin, in the second case, because it needs to).
If the part has both pads, then the switch will be open by default. In the af.csv we configure just Pxy, with just the ADC units and channels available to Pxy. Then the new ADCBlock allows you to access the Pxy_C channels. The switch always stays open.

@jimmo
Copy link
Member

jimmo commented Nov 1, 2023

Damien suggested that rather than waiting for ADCBlock to exist in order to access the _C channels, today we could add e.g. ADC(ADC.PC2_C) to achieve this. (This matches the existing e.g. ADC.CORE_TEMP constants to access other non-pin channels)

So in the af.csv, the PC2 line for example would be PC2,...,ADC123_INN11/ADC123_INP12/C_ADC3_INN1/C_ADC3_INP0 where the C_ prefix indicates that they are to be made available as ADC constants. There would be no PC2_C line.

This would work the same way ADC(pin.PC_2) works today, ADC(ADC.PC2_C) it takes the first available channel from the C_ prefixed ADC on the PC2 line with the most channels. In the future we can use ADCBlock to access other ADCs & channels.

I will add this to the boardgen.py PR. I think we should keep the second commit from this PR though (with the #if changed to #ifdef), even if it is setting the exact same value as the reset defaults.

@iabdalkader
Copy link
Contributor Author

iabdalkader commented Nov 1, 2023

@jimmo

Would someone ever want to change the switch state at runtime. i.e. a generic board that has both PC2 and PC2_C wired to external headers, there are several combinations available to them.
On the Nicla, would a user ever need/want to open the switch, thereby disconnecting ULPI NXT and DIR pins?

No, this is definitely a board design characteristic, that depends on what those pins were intended to do. If Pxy_C pins were labeled as analog pins on a board, then the switch should be compile-time configured as open. For Nicla Vision, they are used for the ULPI pins, if they're changed the USB will be broken, so the switch should be configured as closed. I don't think those pins should be allowed to change in runtime (not that there's any API to do that anyway).

This would work the same way ADC(pin.PC_2) works today, ADC(ADC.PC2_C) it takes the first available channel from the C_ prefixed ADC on the PC2 line with the most channels. In the future we can use ADCBlock to access other ADCs & channels.

@jimmo @dpgeorge I don't think this needs to be any more complex than it is already, this PR lets you map board pins to dual pads as you normally would in pins.csv, and adds them to the right ADC tables, but if you have something else in mind that you want to implement that's fine.

However note that the sole purpose of this PR is to support mapping board pins to dual pads, for example on Portenta board pin A0 is connected to PA0_C so the following works right now:

adc = ADC("A0")

It's important to use "A0" because it makes examples portable (on Giga board pin A0 is PC4). ADC(ADC.PC2_C) is Not portable. If what you're proposing will still support this somehow, I can just drop the first commit.

@dpgeorge
Copy link
Member

dpgeorge commented Nov 2, 2023

However note that the sole purpose of this PR is to support mapping board pins to dual pads, for example on Portenta board pin A0 is connected to PA0_C so the following works right now:

I see you've configure the switch for this pad to be open. That means the user can only use ADC on this board-level A0 pin, and never any digital IO. Is that the intention?

@jimmo
Copy link
Member

jimmo commented Nov 2, 2023

Replicating @iabdalkader's comment from #12211 here to keep the conversation in one place.


I've added the C_ADC entries to the af.csv files as discussed in #12706.

@jimmo

Damien suggested that rather than waiting for ADCBlock to exist in order to access the _C channels, today we could add e.g. ADC(ADC.PC2_C) to achieve this.

I left a comment on #12706. This feels like a workaround to access those channels, my goal is not just access the extra ADC channels, the main goal is to support mapping "board pins" to Pxy_C pads. For example on Portenta board pin A0 is connected to PA0_C so the following should work:

adc = ADC("A0") # ADC12_IN0 channel

It's important to use A0 because it makes examples portable (on Giga board pin A0 is PC4) ADC(ADC.PC2_C) is Not portable. After the changes here, can we still support that ?

NOTE: This fact that A0 is connected PC2_C is only documented in schematics and datasheets, the user only sees A0 on the board's silkscreen. How would a user know which pad maps to an Ax pin ?

Also, if the above won't be supported, what should I map the following board pins to ? If I map them to their Pxy counterparts, we'll have 3 duplicated channels, if I don't map them at all we'll be missing 3 analog pins (i.e ADC('A8') will raise an exception).

+A4,PC3
+A5,PC2
+A7,PA0

+A8,PC2_C
+A9,PC3_C
+A11,PA0_C

A bigger problem here with the Portenta, those 4 pins their Pxy pads are already connected and used as AFs, so I can't map them to Pxy (and have duplicate channels), I will have to remove them from csv.

+A0,PA0_C
+A1,PA1_C
+A2,PC2_C
+A3,PC3_C

@jimmo
Copy link
Member

jimmo commented Nov 2, 2023

The problem is that ADC("A0") works because the board name A0 maps to a pin that isn't actually a pin. i.e. although this works nicely for ADC, it means that the user can also write machine.Pin("A0") which is confusing -- A0 (mapped to PC2_C) is not actually a pin.

(It's especially confusing that "A0" is a board name in this case, which takes precendence over the cpu name A0... see the other discussion about renaming cpu names to include the "P" prefix).

I see your point about the fact that there needs to be a portable way to be able to use the ADC on the thing on the board labeled "A0".

What if we were to support a special case on H7, where ADC(str), before trying to look up the string as a pin name via pin_find, first tried some special aliases.

So on the portenta, ADC("A0") would be equivalent to ADC(ADC.PC2_C), whereas on the Giga, ADC("A0") would do the usual thing of being equivalent to ADC(Pin.board.A0) (i.e. ADC(Pin.cpu.PC4)).

Also, if the above won't be supported, what should I map the following board pins to ? If I map them to their Pxy counterparts, we'll have 3 duplicated channels,

I think the above suggestion addresses this?

You could have e.g.

A0,ADC.PC2_C
A5,PC2

And with the switch open, ADC("A0") would use the channel connected to the _C pad, and ADC("A5") would use the channel connected to the regular pad.

@jimmo
Copy link
Member

jimmo commented Nov 2, 2023

To put this another way:

  • Using ADCn_INm_C in pins.csv for Pxy causes additional ADC constants ADC.Pxy_C to be generated.
  • You can also reference ADC.Pxy_C in pins.csv, which causes that line not to generate a board & cpu machine.Pin, rather an extra board string alias that can be used in the ADC constructor.

@iabdalkader
Copy link
Contributor Author

I see you've configure the switch for this pad to be open. That means the user can only use ADC on this board-level A0 pin, and never any digital IO. Is that the intention?

Yes that's the intended use, the Pxy pads are already used for different AF functions and can't be allowed to be used as I/O. For example PA1 is connected to the ETH physical interrupt pin, PC1 is connected to MDC, and other pins UART and SPI. Note even on the HD connector those pins are labeled ADC_Ax.

image

@iabdalkader
Copy link
Contributor Author

pins.csv would have A0,ADC.PC2_C which would not emit a board pin or cpu pin, but would allow ADC("A0") to be a portable alternative to ADC(ADC.PC2_C).

@jimmo I'm very happy with this solution, in fact I think it fixes one more issue, as I've mentioned those pins should Not be allowed to be used as GPIOs otherwise they'll break things.

@iabdalkader iabdalkader force-pushed the stm32_dual_pad_pins branch 2 times, most recently from 7873533 to 3274d08 Compare November 2, 2023 10:38
@iabdalkader iabdalkader changed the title stm32: Add support for dual-pad pins and analog switches configuration. stm32: Add analog switches configuration. Nov 2, 2023
Signed-off-by: iabdalkader <i.abdalkader@gmail.com>
@dpgeorge dpgeorge force-pushed the stm32_dual_pad_pins branch from 3274d08 to e5014a4 Compare November 3, 2023 05:30
@dpgeorge dpgeorge merged commit e5014a4 into micropython:master Nov 3, 2023
@iabdalkader iabdalkader deleted the stm32_dual_pad_pins branch November 3, 2023 06:21
@iabdalkader
Copy link
Contributor Author

Is A0,ADC.PC2_C supported in #12211 or is it going to be supported in a different PR ? Also after merging #12211 the ADC pins don't look right, it seems to be adding differential channels to the table, in single ended mode those will be the same channel right ?

    [3] = &pin_A6_obj,
    [3] = &pin_A7_obj,

    [5] = &pin_B0_obj,
    [5] = &pin_B1_obj,

    [10] = &pin_C0_obj,
    [10] = &pin_C1_obj,

@jimmo
Copy link
Member

jimmo commented Nov 3, 2023

@iabdalkader Different PR, which I had just started on and was looking at the same thing.

The main issue is that it used to suppress cpu-hidden (i.e. X,-Y lines in pins.csv) from the adc tables.

That part is easy to fix. The output should be:

const machine_pin_obj_t * const pin_adc1[19] = {
    [16] = &pin_A0_obj,
    [16] = &pin_A1_obj,
    [18] = &pin_A4_obj,
    [11] = &pin_C2_obj,
    [12] = &pin_C3_obj,
    [2] = &pin_F12_obj,
};

const machine_pin_obj_t * const pin_adc2[19] = {
    [18] = &pin_A4_obj,
    [11] = &pin_C2_obj,
    [12] = &pin_C3_obj,
    [2] = &pin_F13_obj,
};

const machine_pin_obj_t * const pin_adc3[16] = {
    [11] = &pin_C2_obj,
};

So the problem is the double-up of A0 and A1 to channel 16. I think this worked essentially by accident before (and incorrectly, because it was assigning A0 and A1 to the _C channels). We probably need the logic that chooses a default channel for a pin to be aware that it's already used that channel for a different pin.

(I guess one could argue it was working correctly before because it would have worked to use the _C channels, via the A0 pin, but I think this would still have incorrectly put the pin into ADC mode).

But also I don't think A0,A1,C2,C3 should be in these tables, as you do not use these pins for ADC (only their _C pads). Also these tables are only used for pyb.ADC.

Anyway, I'm working on it now.

@jimmo
Copy link
Member

jimmo commented Nov 3, 2023

@dpgeorge

Ignoring dual-pad pins for now, what should the following config in af.csv do:

PortA,PA0,...,ADC1_INP16
PortA,PA1,...,ADC1_INN16/ADC1_INP17

i.e. I should be able to do both machine.ADC(machine.Pin.cpu.PA0) and machine.ADC(machine.Pin.cpu.PA1) and use them independently.

Right now it will put both on ADC1 channel 16 (under the criteria that make-pins.py has always used -- first channel entry with the greatest number of units).

Should we try and put them on different channels (in which case channel 16 will be excluded for PA1), or should we always take only the P channels? (I think that there is at most 1 P or N channel for a given pin?)
(Maybe that's what the existing logic did... I cannot seem to figure out what the intention was there?)

@jimmo
Copy link
Member

jimmo commented Nov 3, 2023

I think I answered my own question... the previous code only got the P channels.

@iabdalkader
Copy link
Contributor Author

The main issue is that it used to suppress cpu-hidden (i.e. X,-Y lines in pins.csv)

Yes this particular issue was discussed somewhere here before and fixed at some point after 1.20 I think.

I think I answered my own question... the previous code only got the P channels.

That would make sense, the stm32 ADC driver doesn't support differential mode, ADC_SINGLE_ENDED is hard-coded.

@jimmo
Copy link
Member

jimmo commented Nov 3, 2023

Sent #12867 to fix the immediate issue.

@iabdalkader
Copy link
Contributor Author

Hi @jimmo just wondering if dual pads support is done somewhere and I missed it, or if it's still in the works ?

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.

3 participants