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

Added definitions for Pimoroni RP2040-based boards #4228

Merged
merged 21 commits into from
Feb 23, 2021

Conversation

ZodiusInfuser
Copy link

Definitions are for:

  • Pimoroni Tiny 2040
  • Pimoroni Keybow 2040
  • Pimoroni PicoSystem

I believe I have set all the files up correctly, but am creating this as a draft PR so we can check things over first.

A few questions I couldn't find answers to:

  • Is there a way to specify the amount of flash available on our boards, or does CPy not require this?
  • Some of our LEDs are common anode, so need to be driven low to turn on. Is there a way to invert the pin behaviour globally so doing led.value = True turns the light on, where led is a DigitalIO object.

@dhalbert
Copy link
Collaborator

Thanks and welcome!

Several of your files have no trailing newline. This will cause the pre-commit checks to fail. You can turn on pre-commit locally to catch this before you push.

$ pip3 install pre-commit   # Just do this once.
...
$ pre-commit install        # Do this once in each repo clone.

You can run the pre-commit checks manually: check the help info for pre-commit.

  • Is there a way to specify the amount of flash available on our boards, or does CPy not require this?

This has not yet been parameterized, and if yours are different, then we need to finish this off. Right now it's hardwired here:

// TODO: Parameterize flash size based on the configured flash.
#define TOTAL_FLASH_SIZE 2 * 1024 * 1024

  • Some of our LEDs are common anode, so need to be driven low to turn on. Is there a way to invert the pin behaviour globally so doing led.value = True turns the light on, where led is a DigitalIO object.

We do not have an invert capability like MicroPython Signal (link). We haven't really discussed this. It could be a new arg to DigitalInOut. Whether it change the behavior of .value or we should add new boolean property like .on would be a question. We can open an issue to discuss this in more detail.

In any case the user would need to know which pins need inverted behavior, right? Or does MicroPython provide a way to specify that in a board definition? Perhaps a naming scheme would help, like LED_G_INV ? INV is off the top of my head; you might have a better suggestion.

@ZodiusInfuser
Copy link
Author

Thanks for the reply, and mention about new lines. I thought I'd followed the other board definitions in the folder exactly, but clearly not.

Just tried installing pre-commit, but the second line does not work, as in it cannot find pre-commit. Note I am developing under Ubuntu.

"This has not yet been parameterized, and if yours are different". Yes, I believe both Tiny2040 and PicoSystem have more flash than the default 2MB of the Pico. Also Adafruit's own Feather RP2040 has 4MB according to the product page, but that is yet to be configured.

@ZodiusInfuser
Copy link
Author

Fixed the pre-commit issues with a bit of trial and error, so if my config looks okay and matches the style you're expecting, then I think this is good to turn into a proper PR.

@dhalbert
Copy link
Collaborator

Just tried installing pre-commit, but the second line does not work, as in it cannot find pre-commit. Note I am developing under Ubuntu.

You should have /home/yourusername/.local/bin in your $PATH. If not, that's probably why it can't find pre-commit.

@dhalbert
Copy link
Collaborator

dhalbert commented Feb 20, 2021

Fixed the pre-commit issues with a bit of trial and error, so if my config looks okay and matches the style you're expecting, then I think this is good to turn into a proper PR.

"This has not yet been parameterized, and if yours are different". Yes, I believe both Tiny2040 and PicoSystem have more flash than the default 2MB of the Pico. Also Adafruit's own Feather RP2040 has 4MB according to the product page, but that is yet to be configured.

So we need to provide you a way to specify a larger flash chip, I think? Or can you live with 2MB for now?

Normally we specify the possible flash chips, and our initialization code knows what size they are. The RP2040 is a different because it has to execute from external flash right away. We plan to rework the way we use flash. Are you using a chip from the same family as the Pico or a different manufacturer's chip? Or to be more precise, could you list which chips you are using?

@ZodiusInfuser
Copy link
Author

"So we need to provide you a way to specify a larger flash chip, I think? Or can you live with 2MB for now?"
With the B1 silicon arriving imminently and being put into products, we can live with the 2MB definition for now. Then hopefully in a month we can push an update once the flash changes you mention are in place.

"Are you using a chip from the same family as the Pico or a different manufacturer's chip?" I don't have that information sadly, but I would guess that it's a different flash chip manufacturer.

@dhalbert
Copy link
Collaborator

Sounds fine. Do the current builds indeed work on all your boards?

@ZodiusInfuser
Copy link
Author

ZodiusInfuser commented Feb 20, 2021

I've tested the Tiny2040 to the best of my ability, but don't have the other two boards to hand so will have to get colleagues to test on Monday

@tannewt
Copy link
Member

tannewt commented Feb 20, 2021

I'll factor out the flash size early next week. I don't want to release anything with the 2MB setting because changing it later would require reformatting the file system. We can make the size configurable as a stop-gap before we have settings per flash sku and board.

@tannewt
Copy link
Member

tannewt commented Feb 23, 2021

#4247 has the stop-gap move for flash size. I'll follow up with a better fix after we get some functionality lots of folks are asking for.

@ZodiusInfuser
Copy link
Author

Thanks @tannewt. I've added those definitions to our boards now.

Should I expect to see Windows/Linux report a larger mass storage size than 1MB, or is that not hooked in yet? I've tried both our board firmwares and the new QTPy RP2040 and they all report 1MB free.

If so, and you think everything else on this PR looks good, i'll go ahead and submit it.

Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

Please include (or follow up with) the flash part numbers and how they are connected. I'll need that info for my future refactoring.

@ZodiusInfuser ZodiusInfuser marked this pull request as ready for review February 23, 2021 17:38
@ZodiusInfuser
Copy link
Author

Thanks Scott. Will follow up with flash codes in the near future

Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

Thank you!

@tannewt tannewt merged commit 001d729 into adafruit:main Feb 23, 2021
@tannewt
Copy link
Member

tannewt commented Feb 23, 2021

You'll want to add the boards to https://github.com/adafruit/circuitpython-org too.

@tannewt tannewt mentioned this pull request Feb 23, 2021
10 tasks
@ZodiusInfuser
Copy link
Author

Thanks again Scott. I'll look at the website soon. Does it automatically associate the UF2 with the page? Looks like there's some magic going on under the hood

@tannewt
Copy link
Member

tannewt commented Feb 24, 2021

It should just work as long as the board ids (aka folder name) match between the two repos. Releases here produce a json file that is merged into circuitpython-org with availability info. So, these boards will have a concrete release artifact with the next beta.

@ZodiusInfuser ZodiusInfuser deleted the rp2040-boards branch March 16, 2021 11:06
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

4 participants