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

ARDUINO_ARCH_STM32 should not be defined #684

Merged
merged 1 commit into from Dec 25, 2019

Conversation

fpistm
Copy link
Contributor

@fpistm fpistm commented Sep 24, 2019

The ARDUINO_ARCH_{build.arch} is used to differentiate core.
See: https://github.com/arduino/Arduino/wiki/Arduino-IDE-1.5:-Library-specification#working-with-multiple-architectures

Defining ARDUINO_ARCH_STM32 will raised several issues on library supporting both cores:

  • ARDUINO_ARCH_STM32
  • ARDUINO_ARCH_STM32F1

This was introduced here:
2208bcb

The ARDUINO_ARCH_{build.arch} is used to differentiate core.
See: https://github.com/arduino/Arduino/wiki/Arduino-IDE-1.5:-Library-specification#working-with-multiple-architectures

Defining ARDUINO_ARCH_STM32 will raised several issues on library
supporting both cores:
 * ARDUINO_ARCH_STM32
 * ARDUINO_ARCH_STM32F1

Signed-off-by: Frederic Pillon <frederic.pillon@st.com>
@stevstrong
Copy link
Collaborator

stevstrong commented Sep 24, 2019

The architecture is not identical with core.
As long both cores support the same architecture, there must be other way to differentiate cores.

@fpistm
Copy link
Contributor Author

fpistm commented Sep 24, 2019

@stevstrong
In fact this repo (Arduino_STM32) include 2 cores (2 archs):

  • STM32F1
  • STM32F4

The Arduino_Core_STM32 support all STM32 arch.
That's why you will find in several libraries:

  • Support for thi core under ARDUINO_ARCH_STM32F1 or ARDUINO_ARCH_STM32F4
  • library.properties with architectures= field:
    • architectures=stm32f1 <-- support this repo for STM32F1
    • architectures=stm32f4 <-- support this repo for STM32F4
    • architectures=stm32f1,stm32 <-- support this repo for STM32F1 and Arduino_Core_STM32
    • architectures=* <-- support all arch
    • ...

Ex:
https://github.com/rogerclarkmelbourne/Arduino_STM32/blob/9d46a1c27d4dc9dd4df06b76a1d81684519d8acc/STM32F1/libraries/WS2812B/library.properties

The ARDUINO_ARCH_{build.arch} is well defined in this repo:

recipe.c.o.pattern="{compiler.path}{compiler.c.cmd}" {compiler.c.flags} -mcpu={build.mcu} -DF_CPU={build.f_cpu} -DARDUINO={runtime.ide.version} -DARDUINO_{build.board} -DARDUINO_ARCH_{build.arch} {compiler.c.extra_flags} {build.extra_flags} {compiler.libs.c.flags} {includes} "{source_file}" -o "{object_file}"

The main concerns is to not add extra define to other ARDUINO_ARCH_{build.arch}.

@fpistm
Copy link
Contributor Author

fpistm commented Sep 24, 2019

I see that my link to original commit was not good. I've updated it:
2208bcb

@stevstrong
Copy link
Collaborator

stevstrong commented Sep 24, 2019

As both cores support same architectures, I wold suggest following defines to differentiate between cores:

ARDUINO_CORE_STM - for your (STM official) core

ARDUINO_CORE_LIBMAPLE - for this (or other) Libmaple based core.

@fpistm
Copy link
Contributor Author

fpistm commented Sep 24, 2019

@stevstrong ,
I think you don't understand this issue.
The two cores present in this repo defined well the ARDUINO_ARCH_{build.arch} and follow the Arduino specification. My concerns is only to remove the one which is not related to this core not to define new one which have no interest.
If you want to define it, up to you but we talk about a standard core definition.
If a third library add the support to one of our cores then he have to use the proper ARDUINO_ARCH_{build.arch} as it is already done in several third party library.
If you do not remove the ARDUINO_ARCH_STM32 several libraries could not build due to misalignement.
Example try to build the firmata which support STM core while this repo is not compatible with it.
There are several port done which are properly handled thanks thoses definition.
Cc @rogerclarkmelbourne

@stevstrong
Copy link
Collaborator

So you are saying that

ARDUINO_ARCH_STM32 - defines support for all architecture variants,

in contrast to

ARDUINO_ARCH_STM32F1 and/or ARDUINO_ARCH_STM32F4 - define support for individual architectures?

In this case I think one should replace the actual form with one of these specific ones, not completely remove.

@fpistm
Copy link
Contributor Author

fpistm commented Sep 24, 2019

No...
ARDUINO_ARCH_STM32 is for STM core.
ARDUINO_ARCH_STM32F1 is for this core : https://github.com/rogerclarkmelbourne/Arduino_STM32/tree/master/STM32F1
ARDUINO_ARCH_STM32F4 is for this core:
https://github.com/rogerclarkmelbourne/Arduino_STM32/tree/master/STM32F4

That's all.

It seems you don't know how Arduino proceed to handle cores with architecture.

@stevstrong
Copy link
Collaborator

stevstrong commented Sep 24, 2019

It seems you don't know how Arduino proceed to handle cores with architecture.

You are most probably right.

The Arduino library specification seems to make equivalent of a "core" with the

name of the architecture (as determined by the name of the folder containing it)

Anyway, the define you try to remove with this PR should be rather replaced with ARDUINO_ARCH_STM32F1 for F1 and ARDUINO_ARCH_STM32F4 for F4, I think.

[EDIT]
I got your point, this should avoid double definition of the same architecture.

@stevstrong
Copy link
Collaborator

OK, seems to make sense.
However, by removing this define, one could brake some libraries which already used this define with this core.

@fpistm
Copy link
Contributor Author

fpistm commented Sep 24, 2019

Ah ;)

However, by removing this define, one could brake some libraries which already used this define with this core.

Maybe but in that case the proper way to handle this is to update library with the proper definition ;)
But I doubt there is a lot of libraries using it for this core, in general way library maintainer keep attention to this to follow Arduino guideline. And ARDUINO_ARCH_STM32F1 is really well used.

@fpistm
Copy link
Contributor Author

fpistm commented Dec 11, 2019

FYI, I will do soon a PR to MySensors and as ARDUINO_ARCH_STM32 define is used for this core I will need to stub the variant for this core.

@GitMoDu
Copy link
Contributor

GitMoDu commented Dec 17, 2019

This is something that has bugged for some time, but I didn't put the effort into assessing what's wrong exactly. I was also initially confused with ARDUINO_ARCH_STM32 when I wanted do differentiate support from AVR, with this clean-up, things should be much more clear for library developers.
As for existing libraries using ARDUINO_ARCH_STM32, if they are working now and relying on another "core", it's a miracle they work at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants