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

[variant] Improve variant support and genericity #1091

Merged
merged 81 commits into from
Apr 17, 2021

Conversation

fpistm
Copy link
Member

@fpistm fpistm commented Jun 5, 2020

As the number of variants continues to grow and in order to ease navigation, they have been grouped in sub-folders per
series.

To improve variants support and generic ones several enhancements/fixes has been done:

  • Each variant will be linked to its mcu/package
  • All pins capabilities will be possible without modifying the PeriperhalPins.c PinMap array. ALTx pin naming will allow to use all alternative possibilities which use other HW peripheral instances.
  • All remap pins will be supported (F0/G0)
  • All dual pad pins will be supported (H7)
  • Analog definition clean up
  • Several fixes in existing variants

All generic variants are now automatically generated thanks the STM32_open_pin_data repository which provides all the information required for the pin configuration of products based on STM32 MCU. All the generic variants are generated in the variant folder, this means all the generic STM32 MCU are generated. Only the linker script and the clock config are missing. A board_entry.txt file is generated to ease board declaration.

Migrated STM32 series:

  • STM32F0
  • STM32F1
  • STM32F2
  • STM32F3
  • STM32F4
  • STM32F7
  • STM32G0
  • STM32G4
  • STM32H7
  • STM32L0
  • STM32L1
  • STM32L4
  • STM32L5
  • STM32MP1
  • STM32WB

Issues linked to this PR:

Fix #855, fix #857, fix #1180, fix #1276, fix #1277, fix #1302, fix #1144

PR for new variant included in this one;


One issue has been met with Arduino IDE about the menu scrolling: arduino/Arduino#11416

Wiki will be updated after official 2.0.0 core release

That would be fine to define the variant in a different way, like this for example:

GenYY.build.variant={build.series}/{build.subvariant}
...
Nucleo_144.menu.pnum.NUCLEO_XYZ.build.subvariant=Generix_XYZx

but it does not work. It builds fine until the compile core step. The automatic default include is simply ignored while for other step it works fine.
I guess there is an issue around the way variant is managed in the Arduino IDE.
Any input are welcome.
@matthijskooijman do you have any idea about this issue? --> arduino/arduino-cli#762

@fpistm fpistm added the enhancement New feature or request label Jun 5, 2020
@fpistm fpistm marked this pull request as ready for review June 5, 2020 19:20
@fpistm
Copy link
Member Author

fpistm commented Jun 6, 2020

This change impact PIO and I don't know how to solve this.
@valeros, I'm currently studying how to better handle variants, this PR is one solution I've planned.
How can we handle this correctly with PIO to ensure backward compatibility?

@fpistm fpistm marked this pull request as draft June 9, 2020 15:35
@fpistm
Copy link
Member Author

fpistm commented Jun 24, 2020

@matthijskooijman

I saw in 1.8.13 your rework of the boards menu per architecture arduino/Arduino#9238

has you any though on this PR and this question ?

That would be fine to define the variant in a different way, like this for example:

Nucleo_144.build.variant=Nucleo_144/{build.subvariant}
...
Nucleo_144.menu.pnum.NUCLEO_F207ZG.build.subvariant=NUCLEO_F207ZG

but it does not work. It builds fine until the compile core step. The automatic default include is simply ignored while for other step it works fine.
I guess there is an issue around the way variant is managed in the Arduino IDE.
Any input are welcome.
@matthijskooijman do you have any idea about this issue?

I think about open an issue about that or a question on the devel group list.

The way to define a board/variant does not allows enough possibilities and seems very restricted. I guess it would be fine to allows build submenu more easily and have a variant path more "dynamic"

@valeros
Copy link
Contributor

valeros commented Jun 24, 2020

Thanks for keeping me informed. We're going to update STM32 platform with the v1.9.0 release, so we can take into account possible future changes of variant paths. Is it already decided to introduce an intermediary folder
variants/ARMED_V1/PinNamesVar.h → variants/3dprinter/ARMED_V1/PinNamesVar.h?

@fpistm
Copy link
Member Author

fpistm commented Jun 25, 2020

Thanks for keeping me informed. We're going to update STM32 platform with the v1.9.0 release, so we can take into account possible future changes of variant paths. Is it already decided to introduce a intermediary folder
variants/ARMED_V1/PinNamesVar.h → variants/3dprinter/ARMED_V1/PinNamesVar.h?

Hi @valeros

currently this is under investigation. 1.9.0 is not impacted.
Could you give me some tricks how to manage this during my testing?

@valeros
Copy link
Contributor

valeros commented Jun 25, 2020

Probably the best way is to fork platform-ststm32 and change variant field in the board manifests that used in CI checks blackpill_f103c8 remram_v1), e.g.:

{
  "build": {
    "core": "stm32",
    ...
    "variant": "ARMED_V1"
  },
  ....
}
{
  "build": {
    "core": "stm32",
    ...
    "variant": "3dprinter/ARMED_V1"
  },
  ....
}

@matthijskooijman
Copy link
Contributor

matthijskooijman commented Jun 25, 2020

@fpistm, that {build.subvariant} does not work seems to be a limitation or bug in the arduino-cli code (which is used by the Arduino IDE via arduino-builder). I reported a bug, look there for more details: arduino/arduino-cli#762

A workaround would be what you've done now: Just repeat the group path in each menu option. Not elegant, but at least it works. Maybe if Arduino fixes this, it can be changed later. I can't comment on PlatformIO, but I think @valeros can :-)

Is this what you wanted feedback on? Or is there any other question I missed?

@fpistm
Copy link
Member Author

fpistm commented Jun 25, 2020

Thanks @matthijskooijman !!

Note that the compiler command has no -I option for the variant when compiling the core (but it is present for the sketch).

Yes that's what I saw during my test. For sketch it works but not for compiler while with the method used in this PR the -I option is available with the correct path.

In fact when I saw your rework of the menu, I'm thinking about why we do this kind of boards.txt with the boards group and then the real variant. This is due to the fact it is not possible to construct a submenu.

What would be fine would be to be able to construct a menu with submenu easily and then also be able to link upload method per submenu to avoid duplicate upload boards entries for each group.

Anyway, thanks again.

Just FYI, I've an empty menu with Roger's core. I guess this is due to empty boards.txt in drivers and tools folders:
image

https://github.com/rogerclarkmelbourne/Arduino_STM32/blob/master/drivers/boards.txt
https://github.com/rogerclarkmelbourne/Arduino_STM32/blob/master/tools/boards.txt

Maybe this file can now be safely deleted or simply Arduino should ignore empty file?

@matthijskooijman
Copy link
Contributor

In fact when I saw your rework of the menu, I'm thinking about why we do this kind of boards.txt with the boards group and then the real variant. This is due to the fact it is not possible to construct a submenu.

What would be fine would be to be able to construct a menu with submenu easily and then also be able to link upload method per submenu to avoid duplicate upload boards entries for each group.

Well, reworking the nested menu was easy, that was just GUI code, it did not require changes to the boards.txt interpretation... Reading what you wrote, I thought it could be nice to allow grouping of boards (e.g. to have submenus within the "Boards" -> "STM32..." menu). However, that would probably only cause more duplication of stuff in boards.txt rather than less.

This is because the extra menus are now always defined per-board, so there is no way to have common (per-core) menus. I've realized this would be a good feature to add, but I haven't really sat down to think about this, let alone implement it (and it changes semantics of boards.txt, so that would not be possible in a backward-compatible manner).

Just FYI, I've an empty menu with Roger's core. I guess this is due to empty boards.txt in drivers and tools folders:

Ah, that's a completely separate issue that is triggered by my nested boards change. I'm not sure what the best fix for this is, since Roger's core does sort of abuse the system by providing an empty boards.txt. Maybe you should file a bug at Roger's repository to further discuss this, since it's a bit off-topic for this issue.

@fpistm
Copy link
Member Author

fpistm commented Jun 25, 2020

Ah, that's a completely separate issue that is triggered by my nested boards change. I'm not sure what the best fix for this is, since Roger's core does sort of abuse the system by providing an empty boards.txt. Maybe you should file a bug at Roger's repository to further discuss this, since it's a bit off-topic for this issue.

As said it is just for your interest and of course off-topic ;)

However, that would probably only cause more duplication of stuff in boards.txt rather than less.

OK. Just a thought as I think this could be probably better managed. Anyway, I guess this would be really useful only for this core. We planned to deployed more generic variant and so this will add a lot of entry in the boards.txt. Hope there is no limit in Arduino IDE (even the new pro IDE) and also with arduino-cli.

@fpistm fpistm added this to the 2.0.0 milestone Jul 8, 2020
@fpistm fpistm force-pushed the subvariant branch 3 times, most recently from 1e544ef to 38f6145 Compare July 10, 2020 09:56
@fpistm
Copy link
Member Author

fpistm commented Jul 10, 2020

Thanks @valeros for the hints. I've made the patch in the GH action and it works well:
38f6145

I wonder if it could be possible to manage backward compatibilities in the script which manages the variant JSON entry to test if the path exists or not ?
Maybe having in the boards JSON:

    "variant": "Generic_F1/PILL_F103XX",
    "alt_variant": "PILL_F103XX",

@valeros
Copy link
Contributor

valeros commented Jul 10, 2020

@fpistm I think we can handle this is in the build script instead of adding a new filed alt_variant to the board manifests. If the path Generic_F1/PILL_F103XX doesn't exist, then we can use only the latter PILL_F103XX.

@fpistm
Copy link
Member Author

fpistm commented Jul 10, 2020

Thanks @valeros
This would be fine and ensure backward compatibility.

@fpistm fpistm changed the title [variant] Group each variant per boards type [variant] Group each variant per boards type or manufacturers Jul 10, 2020
@fpistm fpistm marked this pull request as ready for review July 10, 2020 14:16
@fpistm fpistm marked this pull request as draft July 10, 2020 14:53
@ABOSTM
Copy link
Contributor

ABOSTM commented Jul 10, 2020

I think that having 2 ways of categorization at the same level (by manufacturer and by board type) may be ambiguous, there may be some overlap. What to do when manufacturer XXX introduce a 3D printer board ?
So I would prefer only 1 rule, or several rules at several level (ex manufacturer 1st level, board type second level).

That said, I would be in favor of 1st level being STM32 series (F1, F4, ...), this could help future enhancement where a board inherit from Generic implementation.

@fpistm
Copy link
Member Author

fpistm commented Jul 10, 2020

Thanks for you feedback @ABOSTM
This make sense as the core is more MCU oriented.
I guess this need more investigation and probably this will help to factorize boards definitions, mainly for generic ones.

Any feedback are welcome. 😉

ABOSTM and others added 8 commits April 16, 2021 15:09
Note this user button is not available on all revisions of this board

Fixes stm32duino#1144

Signed-off-by: Alexandre Bourdiol <[email protected]>
Supersed stm32duino#1130

Signed-off-by: dds90 <[email protected]>
Co-authored-by: Alexandre Bourdiol <[email protected]>
Invert PF9 and PF10 in digitalPin[] to match their definition in
variant.h
Fixes forum issue:
https://www.stm32duino.com/viewtopic.php?p=6652#p6652

Signed-off-by: Alexandre Bourdiol <[email protected]>
Signed-off-by: Alexandre Bourdiol <[email protected]>
Supersed stm32duino#1208

Signed-off-by: Martin Cerný <[email protected]>
Co-authored-by: Alexandre Bourdiol <[email protected]>
This is is only a workaround to allow the PIO build action.
Targets have been changed due to '(' and ')' in their paths
which prevent the build even if they are protected.

Signed-off-by: Frederic Pillon <[email protected]>
Wiki will be updated to describe the new way to define a variant.

Signed-off-by: Frederic Pillon <[email protected]>
This new function could be called by STM32duino_Low_Power library

Signed-off-by: Alexandre Bourdiol <[email protected]>
Currently only H7 is concerned.
MP1 hardware also have dualpad analog switch but behavior is different
and in our case, switch should always remain open.

Signed-off-by: Alexandre Bourdiol <[email protected]>
Signed-off-by: Frederic Pillon <[email protected]>
@fpistm fpistm merged commit eaeb794 into stm32duino:master Apr 17, 2021
@fpistm fpistm deleted the subvariant branch April 17, 2021 12:44
@matthijskooijman
Copy link
Contributor

\o/

@fpistm
Copy link
Member Author

fpistm commented Apr 17, 2021

Finally done!

@dreamcat4
Copy link

that was some huge merge. well done! very good work.

sorry if this is wrong place to ask. but had a simple little question (linked over on other issue), just if anybody here happens to know the answer

Serasidis/STM32_HID_Bootloader#23 (comment)

if these newly commited improvement can... lets me flash an hid bootloader and memory magic for flashing mode... without manually be pressing the reset button to reboot it every time. i think it means some special signal has to be sent over HID usb connection. to interrupt the running userland program, ad jump into the hid bootloader. which can then do all the flashing, reboot etc.

because for the life of me... i cannot find the answer to this question! yet the weact black pill f4xx is a very popular board. i searched and searched

@fpistm
Copy link
Member Author

fpistm commented Apr 17, 2021

@dreamcat4

Thanks.

For HID. I saw several issue linked to the WeAct HID fork. I don't know why so many issue... Anyway, I do not use it and HID seems no more maintained by @Serasidis.
I have to end #710 which is more generic (except for F1 which doesn't have USB support in builtin bootloader) and do not need extra stuff.

@avtoneru
Copy link

avtoneru commented May 16, 2021

Wiki will be updated after official 2.0.0 core release

When can we expect an update wiki?

@ABOSTM
Copy link
Contributor

ABOSTM commented Aug 2, 2021

@ridvanmelih

I looks like there is something wrong with github:
I receive a email notification about the following comment

Do we have a chance to define the SPI2 pins here?

related to this commit (part of this PR)

Re: [stm32duino/Arduino_Core_STM32] Add all generated STM32F1xx generic variant files (6b2b23d)

But I could not found the comment in github.
Consequently I don't know the context of this comment

  • which directory is concerned
  • which file is concerned
  • which line is concerned
  • which MCU/board is concerned

What I can tell is that generic files are automatically generated, and all supported spi instances are made available.
So maybe the concerned MCU doesn't have spi2, or you are on a board/MCU which doesn't output spi2 pins

@ridvanmelih
Copy link

ridvanmelih commented Aug 3, 2021 via email

@ABOSTM
Copy link
Contributor

ABOSTM commented Aug 3, 2021

As you can see, SPI1 and SPI2 pins are already defined:

Arduino create a default SPI instance (instance of the class)
In variant_generic.h you can see that default SPI instance used is SPI1 with PA5, PA6, PA7

If you need both SPI, you just need to declare, in you sketch, a new instance with the pins of SPI2 (hardware instance)
For example, (check the pin match your board):

SPIClass SPI_2(PB_15, PB_14, PB_13);
SPI_2.begin();

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arduino compatibility enhancement New feature or request fix 🩹 Bug fix new variant Add support of new bard
Projects
None yet