Thanks, this looks OK to me.

@iabdalkader do you have any comment/objection to this change?

Hi, no objections it looks good. Note I'm also going to add the Arduino pins (A0...Ax, D0...Dx) to all Arduino boards, I already have the change locally but just for the Giga, if you want to add them here as well that's fine.

Hi, no objections it looks good. Note I'm also going to add the Arduino pins (A0...Ax, D0...Dx) to all Arduino boards, I already have the change locally but just for the Giga, if you want to add them here as well that's fine.

I'm game to take a stab at this in a subsequent PR. I looked at doing it in this one, but had two things I wasn't sure how to handle (which may be due to my lack of understanding):

• There would seem to be name collisions when instantiating, for example, Pyb.Pin('A1'). Right now that would refer to the cpu pin. The two 'A1' pins could be accessed separately with Pyb.Pin.[board|cpu].A1, but could this lead to confusion?
• The SPI1_xxx pins that Arduino labels actually utilize the SPI2 bus on the H7, with a non-default pin set. Again, I'm wondering if this would lead to confusion from exposing the SPI2 bus with pins labeled SPI1. Worth noting, the AFs for these pins don't include the SPI1 bus.

• There would seem to be name collisions when instantiating, for example, Pyb.Pin('A1'). Right now that would refer to the cpu pin.

I don't think so, isn't it PA0 ?

The SPI1_xxx pins that Arduino labels actually utilize the SPI2 bus on the H7

Yes that's right, it might be a documentation error will pass that along.

I don't think so, isn't it PA0 ?

PA0 worked because previously pins.csv contained lines like
PC2,PC2
This definition means pyb.Pin.board.PC2 was the same as pyb.Pin.cpu.C2, which is problematic because this breaks the mapping of board pin names from cpu pin names. After this PR the same line becomes:
,PC2
and this line is added:
J2_74,PC2
As a result, no 'PC2' designation exists, but the pin may be accessed with the board designation J2_74 or cpu designation C2.

I see, yes this can be an issue. I think the P should have never been removed from CPU pin names, because it's listed this way in the datasheet, @jimmo any thoughts ?

I think the P should have never been removed from CPU pin names, because it's listed this way in the datasheet, @jimmo any thoughts ?

From the very beginning of the stm port and the pyboard hardware, CPU pins have had their "P" prefix removed. This was done for brevity and to save code space. But, yes, it has lead to some awkward situations where the board name (like "A0") will clash with the CPU name when the P is absent.

But, at least it's still possible to disambiguate by explicitly using the namespace, either Pin.board or Pin.cpu.

If this change is going to be merged (I think it should be), then the other two Arduino stm32 boards will also need to have their pins.csv updated in the same way.

@iabdalkader do you agree with that? Shall we try to do it together in one go?

I'm just not sure anymore why this change is needed. With the current pins.csv, both Pin("PA0") and Pin("A0") work fine and I get the right pin in each case, moreover the correct pin name is used ( "PA0" Not "A0") which matches the datasheets and schematics, but if this is merged, Pin("Pxy") would stop working, and we have to use pyb.Pin.cpu.x to access it, breaking a lot of existing scripts.

Note I do like adding the HD pins that should definitely be merged, and we should also add all A0..Ax and D0..Dx pins for all boards.

As a prerequisite to adding the missing A0..Ax and D0...Dx board pins, I pushed this #12706 which fixes the support for Pxy_C pins. If you want to you can also add them in this PR. Note it's easier to just copy those pins from Arduino's mbed core (from variant.cpp files). For example, for Portenta : https://github.com/arduino/ArduinoCore-mbed/blob/main/variants/PORTENTA_H7_M7/variant.cpp#L24

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:    +0 +0.000% standard
      stm32:    +0 +0.000% PYBV10
     mimxrt:    +0 +0.000% TEENSY40
        rp2:    +0 +0.000% RPI_PICO
       samd:    +0 +0.000% ADAFRUIT_ITSYBITSY_M4_EXPRESS

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.36%. Comparing base (5114f2c) to head (8d83896).

❗ Current head 8d83896 differs from pull request most recent head b02b1cc. Consider uploading reports for the commit b02b1cc to get more accurate results

@@            Coverage Diff             @@
##           master   #12676      +/-   ##
==========================================
- Coverage   98.39%   98.36%   -0.04%     
==========================================
  Files         161      159       -2     
  Lines       21200    21090     -110     
==========================================
- Hits        20860    20745     -115     
- Misses        340      345       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

I'm just not sure anymore why this change is needed. With the current pins.csv, both Pin("PA0") and Pin("A0") work fine and I get the right pin in each case, moreover the correct pin name is used ( "PA0" Not "A0") which matches the datasheets and schematics, but if this is merged, Pin("Pxy") would stop working, and we have to use pyb.Pin.cpu.x to access it, breaking a lot of existing scripts.

Yes, that is true. The current state of this PR is that Pin("Pxy") will no longer work.

Is this too much of a breaking change? Probably. And as mentioned above, if the change is made for Portenta H7, a similar change should be made for the other Arduino stm32 boards.

So, probably what we should do is just merge the high-density connector pin definitions in this PR.

Note that Dx and Ax pin names were added in 32623d3

Is this too much of a breaking change? Probably. And as mentioned above, if the change is made for Portenta H7, a similar change should be made for the other Arduino stm32 boards.

Yes it is, it could break examples, tutorials, docs, user scripts etc... But besides that, I just don't see a point in removing this workaround, it won't save much flash.

So, probably what we should do is just merge the high-density connector pin definitions in this PR.

I agree the HD pins are good to have.

it won't save much flash

Named pins actually take quite a lot of flash. In this case removing them would save 2700 bytes.

That doesn't really matter for the Portenta though, because it has a large flash and already a >1MByte firmware. But in general it's best not to add pin names if they aren't needed.

I agree the HD pins are good to have.

@loopback can you please undo the removal of the named pins, and leave just the high-density pin name additions?

In this case removing them would save 2700 bytes.

It might be worth looking into a proper fix at the port level, in C, to remove them but still keep support Pxy somehow.

It might be worth looking into a proper fix at the port level, in C, to remove them but still keep support Pxy somehow.

Yes, agreed.

See related #13664.