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

Add Thing Plus RP2040 Board #900

Merged
merged 2 commits into from Aug 18, 2022
Merged

Conversation

cocasema
Copy link

No description provided.

@cocasema cocasema force-pushed the thing-plus-rp2040 branch 2 times, most recently from 08c289f to ab9deda Compare August 18, 2022 05:20
Copy link
Member

@salkinium salkinium left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

src/modm/board/thingplus_rp2040/board.hpp Outdated Show resolved Hide resolved
Comment on lines +54 to +55
env.copy(repopath("tools/openocd/modm/rp2040_picoprobe.cfg"), "rp2040_picoprobe.cfg")
env.collect(":build:openocd.source", "modm/board/rp2040_picoprobe.cfg")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about moving the programmer configuration into its own module, since it is not part of the actual devboard?

The RP2040-Picoprobe module can still be included in the blink example in project.xml.

(This should of course also apply to all existing (STM32) Blue-Pill/Black-Pill/... Dev-Board without integrated programmer, but that's out of the scope of this pull request.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not super convinved, since it's just a single file, so seems weird to have it's own module for it.
Maybe it would make more if we had some "debug" module that's more separate from the build module, but that's kinda weird too.
If you write your own non-BSP based application, then you need to copy this file manually anyways?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I always found it annoying in the past with boards like the Blue-Pill to have a different programmer (e.g. stlink v2 vs. stlink v3) than the one assumed in the BSP for no reason and then have to manually patch around in the generated code each time.

it's just a single file, so seems weird to have it's own module for it.

I do not see any disadvantage?
Probably the module should also add the logging functionality to the board (if the programmer has a integrated USB VCP channel) and then document the necessary wiring.

Maybe it would make more if we had some "debug" module that's more separate from the build module, but that's kinda weird too.

modm:debug module exists, and contains modm::log::Logger. Why is it called debug?

If you write your own non-BSP based application, then you need to copy this file manually anyways?

Or just include the programmer module, if that is a module?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have to manually patch around in the generated code each time.

Yeah, that's really annoying. But does specifying the programmer in the openocd.cfg file not work as documented in the Blue Pill docs?

Probably the module should also add the logging functionality to the board (if the programmer has a integrated USB VCP channel) and then document the necessary wiring.

Ok, I like that.

modm:debug module exists, and contains modm::log::Logger. Why is it called debug?

Good old historic reasons of course. I would move it to modm:io:logger.

Or just include the programmer module, if that is a module?

Sure, but if the BSP already includes the programmer module, then you cannot uninclude it again. So you're back to square one?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not opposed to this at all btw, I think it's a really good idea to separate the programmer out, it just needs to be done well as per usual ;-P

@salkinium salkinium merged commit 874c8d6 into modm-io:develop Aug 18, 2022
@cocasema cocasema deleted the thing-plus-rp2040 branch August 19, 2022 16:32
@salkinium salkinium added this to the 2022q3 milestone Aug 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants