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

Makefile*: Allow external CPU dirs #20447

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

david-vankampen
Copy link
Contributor

Contribution description

Following the patterns set by EXTERNAL_BOARD_DIRS, establish an optional EXTERNAL_CPU_DIRS parameter. This allows CPU definitions to reside outside the RIOT code tree.

This change also commonizes the repeating pattern of $(RIOTCPU)/$(CPU) into a more succinct $(CPUDIR).

Testing procedure

In a separate code base, set the EXTERNAL_CPU_DIRS variable, define a new CPU in that directory, and point a BOARD at that CPU, and build an application.

Issues/PRs references

N/A

@github-actions github-actions bot added Platform: ARM Platform: This PR/issue effects ARM-based platforms Area: build system Area: Build system Area: drivers Area: Device drivers Area: tools Area: Supplementary tools Area: boards Area: Board ports Platform: ESP Platform: This PR/issue effects ESP-based platforms Platform: RISC-V Platform: This PR/issue effects RISC-V-based platforms Area: cpu Area: CPU/MCU ports Area: Kconfig Area: Kconfig integration labels Mar 5, 2024
@maribu maribu added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 5, 2024
@riot-ci
Copy link

riot-ci commented Mar 5, 2024

Murdock results

✔️ PASSED

f00e385 Makefile*: Allow external CPU dirs

Success Failures Total Runtime
10009 0 10009 07m:52s

Artifacts

@maribu
Copy link
Member

maribu commented Mar 5, 2024

From a quick peek, this looks good to me.

To be honest, I'd much rather see people upstreaming CPU code - I don't think that drivers are were the secret sauce of a firmware is and upstreaming them helps everyone.

But then again, an OS should enable the users to do what they want, no matter what @maribu's personal feelings are on that.

And I guess this could be handy while working with experimental RIOT ports that are not yet ready for upstreaming.

I'm sadly afk for the most part of the next two weeks, so I can't do any testing now. Also, I tent to overlook things on a tiny screen that I would notice on a big monitor, so I don't feel comfortable to ACK this right now. But feel invited to ping me again in two weeks, if this has not been approved by then anyway.

@benpicco
Copy link
Contributor

benpicco commented Mar 6, 2024

I agree with @maribu - why not upstream the CPU code?
It's true that upstreaming comes with additional work, but you will benefit in the long run as you don't have to manually keep up with API changes.

That said, we wouldn't make any API grantees for 'external' CPU ports in the first place.

@david-vankampen
Copy link
Contributor Author

why not upstream the CPU code?

I am working with a "custom" native CPU where the IO from peripherals (gpio, i2c, spi, adc) is routed in a custom manner. I don't think it is particularly interesting or useful to other users of upstream RIOT, so I think my first preference would be to manage it "on the side", which this change allows. In this case, there is not a strong need to "keep up with API changes".

That said, we wouldn't make any API grantees for 'external' CPU ports in the first place.

I agree - while the use-cases may be few, or obtuse, there seems to be little harm in supporting this capability, and it makes the CPUs more congruous with BOARDS and PACKAGEs, which have their own notion of "external."

@kfessel
Copy link
Contributor

kfessel commented Mar 14, 2024

I thinks this fits well within out EXTERNAL_... scheme and might also help while developing new cpu that will go upstream later or as the author test some native thingy with something changed up (use parallelport as gpio?, USB-gpios? , firmata-gpio? (there are other things then gpio))

but maybe we should add warning / info /message that asks the user of any of our EXTERNAL_... features to think about upstreaming his code (just a little).

@maribu
Copy link
Member

maribu commented Mar 14, 2024

but maybe we should add warning / info /message that asks the user of any of our EXTERNAL_... features to think about upstreaming his code (just a little).

I do like the idea, but I hate the proposed implementation.

<rant>
The reasoning I am using OpenOCD with my J-Link debuggers is 80% to avoid the harassment of the warning that I am legally bound to use the embedded J-Link programmer that is soldered onto an NXP devboard and physically hardwired to the NXP MCU on that devboard only with NXP MCUs. (As if buying expensive NXP dev boards just to harvest the debuggers from them would be attractive to in the first place...) Being interrupted in my work-flow to be lectured by software really drives me nuts.
</rant>

I really don't want to pull a SEGGER here and start harassing our users with warnings as well. And I don't think that bulling people into upstreaming their code won't work anyway.

How about instead revisit the documentation of the external directories, group that properly (now that there are options to use external boards, modules, packages, and soon CPUs), and add a friendly pointer there with a few reasons why upstreaming can be beneficial and when we would encourage upstreaming code?

@david-vankampen
Copy link
Contributor Author

@maribu would you prefer that documentation update to happen on this PR, or separately?

@maribu
Copy link
Member

maribu commented Mar 15, 2024

If you want to do that in this PR, I would very grateful.

However, I don't think this needed for this PR to get in. That documentation cleanup was needed regardless of this PR. So if you don't want to do that, I can create a follow up.

@github-actions github-actions bot added the Area: doc Area: Documentation label Mar 15, 2024
@david-vankampen
Copy link
Contributor Author

I've added a blurb to the creating-an-application.md near similar documentation on external boards and external modules.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: boards Area: Board ports Area: build system Area: Build system Area: cpu Area: CPU/MCU ports Area: doc Area: Documentation Area: drivers Area: Device drivers Area: Kconfig Area: Kconfig integration Area: tools Area: Supplementary tools CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms Platform: ESP Platform: This PR/issue effects ESP-based platforms Platform: RISC-V Platform: This PR/issue effects RISC-V-based platforms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants