Skip to content

Adding CI build action (and fix usage of deprecated SPI APIs)#1

Closed
aentinger wants to merge 2 commits intomasterfrom
unknown repository
Closed

Adding CI build action (and fix usage of deprecated SPI APIs)#1
aentinger wants to merge 2 commits intomasterfrom
unknown repository

Conversation

@aentinger
Copy link
Copy Markdown

This repository was flagged during semi-automatic review for using any of the SPI APIs setBitOrder, setDataMode, setClockDivider. Unfortunately those SPI APIs are only supported within ArduinoCore-avr and deprecated everywhere else.

This is problematic because the library.properties of this library indicates that any architecture is supported (architectures=*) as it raises expectations by the user that simply can not be fulfilled when using this library on any platform that does not support those SPI APIs.

By donating this PR I'm hoping to start a conversation of either limiting the list of supported architectures to avr

-architectures=*
+architectures=avr

or to adapt the code to use the SPISettings API supported in all cores.

@jaenrig-ifx
Copy link
Copy Markdown
Member

Hi @aentinger,

Thanks for your contribution. We will plan to refactor in terms of SPISetting.

For now, as this is actually also compatible with other third party cores, as the "XMC-for-Arduino"... setting it to avr would not be just right. We could set it to architectures=avr, xmc. But equally it might be the case that other third party cores also keep the deprecated interface.
Is there a way to specify not supported? Something like architecture=*, !samd ?
That would be more inclusive and easier to maintain, as the only alternative to that is to check against all official and third-party core, and list of all them.

@aentinger
Copy link
Copy Markdown
Author

Hi @jaenrig-ifx 👋

Is there a way to specify not supported? Something like architecture=*, !samd ?

To my knowledge this is not supported, but I'll forward your concern to our tooling team. CC @cmaglie @per1234

But equally it might be the case that other third party cores also keep the deprecated interface.

Not as much as you might think. And we can automatically (through CI) test which cores and which libraries use those deprecated APIs. (I did not find this repository by manually going through all of the currently 5876 Arduino libraries 😉 ).

@per1234
Copy link
Copy Markdown

per1234 commented May 17, 2023

Hello. As @aentinger explained, at the moment you can only specify compatibility with all architectures (*) or individual architectures. However, there is work in progress to add something like it, as described here:

arduino/arduino-cli#2149

@jaenrig-ifx
Copy link
Copy Markdown
Member

Thanks @aentinger and @per1234 for your insight and support :)

I will for now update the library.properties architectures to avr, xmc of this library (and this one) and plan a future refactor for using SPISettings and revert to *.

I close the pull request and plan as well the ci workflow inclusion after the SPISettings refactor.

@jaenrig-ifx
Copy link
Copy Markdown
Member

done!

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.

3 participants