Skip to content

ports/rp2: Added init and reset C hooks for RP2040, and increased external pin count #12283

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

Closed
wants to merge 2 commits into from

Conversation

ZodiusInfuser
Copy link
Contributor

Following discussions in https://github.com/orgs/micropython/discussions/12116, this pull request makes changes primarily to support the upcoming Pimoroni Yukon board, but that could also benefit others in the community.

  • Adds C hooks for initialising and resetting an RP2040 based board. Yukon currently only uses a few of these, but I included the rest for future product use.
  • Increases the number of external IO pins from 10 to the maximum 32 to match the number of pins Yukon's IO expanders expose.

I have tested these successfully with a custom build, that includes the board definitions and pulls in drivers for the IO expanders so the external pins can work. I have left these changes out of this PR, as they are currently quite intrusive.

@ZodiusInfuser ZodiusInfuser changed the title Added init and reset C hooks for RP2040, and increased external pin count ports/rp2: Added init and reset C hooks for RP2040, and increased external pin count Aug 22, 2023
@ZodiusInfuser ZodiusInfuser force-pushed the rp_init_hooks branch 3 times, most recently from 2a857c3 to b88262c Compare August 22, 2023 20:02
@Gadgetoid
Copy link
Contributor

Could there be a means to include a C header, for hook code that requires it? Or some knack to it?

In my case - for PicoSystem - I might want MICROPY_BOARD_STARTUP to be:

// Apply a modest overvolt, default is 1.10v.
// this is required for a stable 250MHz on some RP2040s
vreg_set_voltage(VREG_VOLTAGE_1_20);
sleep_ms(10);
set_sys_clock_khz(250000, true);

But that depends upon:

#include "hardware/vreg.h

But yeah, +1 to this change, it could benefit PicoSystem and potentially PicoVision.

@ZodiusInfuser
Copy link
Contributor Author

ZodiusInfuser commented Aug 24, 2023

I think that may already be covered by this addition, as I do

#include "hardware/gpio.h"
#include "hardware/i2c.h"

within my Yukon build. See: https://github.com/pimoroni/yukon/blob/main/firmware/PIMORONI_YUKON/board.c

The functions in this file are hooked in by:

#define MICROPY_BOARD_EARLY_INIT board_init
void board_init(void);

#define MICROPY_BOARD_EARLY_RESET board_reset
void board_reset(void);

@ZodiusInfuser
Copy link
Contributor Author

Hi. Just wondering if anyone had a chance to look at this yet?

Copy link
Contributor

@projectgus projectgus left a comment

Choose a reason for hiding this comment

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

Hi @ZodiusInfuser,

Thanks for being patient while I got around to looking at this.

I think these are useful changes, and thanks for contributing them.

The max EXT_PINs thing is good to go (I was initially cautious until I realised this only raises the maximum to 32, so doesn't change anything on the current boards!)

For the new C hooks, these are great to have on rp2. My only concern is consistency between ports. See the inline comments for specific thoughts.

@ZodiusInfuser
Copy link
Contributor Author

HI @projectgus,

Thank you for taking a look at this.

The max EXT_PINs thing is good to go (I was initially cautious until I realised this only raises the maximum to 32, so doesn't change anything on the current boards!)

Glad to hear it!

For the new C hooks, these are great to have on rp2. My only concern is consistency between ports. See the inline comments for specific thoughts.

I hadn't specifically spotted the STM etc hooks, so apologies for their inconsistency. I will have a look and see what can be changed and respond to your specific notes later.

I will say, I am only actively using the below hooks in the product this PR was primarily raised for.

#define MICROPY_BOARD_EARLY_INIT board_init
#define MICROPY_BOARD_EARLY_RESET board_reset

The others were included to account for possible future use-cases, so I agree they should be consistent.

Copy link

github-actions bot commented Apr 29, 2024

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:    +0 +0.000% standard
      stm32:    +0 +0.000% PYBV10
     mimxrt:    +0 +0.000% TEENSY40
        rp2:    +0 +0.000% RPI_PICO_W
       samd:    +0 +0.000% ADAFRUIT_ITSYBITSY_M4_EXPRESS

@projectgus
Copy link
Contributor

@ZodiusInfuser Are you still working on this PR? Is there anything you're waiting on from the MicroPython side?

@projectgus projectgus removed their assignment Sep 3, 2024
@ZodiusInfuser
Copy link
Contributor Author

ZodiusInfuser commented Sep 3, 2024

Errr... I'll admit I completely forgot about this open PR after our Pimoroni Yukon was launched 😅. It is still necessary as that product's builds still rely on my rp_init_hooks branch.

Looks like I never addressed your review points before. I'll try and find time this week to look at it, if not it will be after I am back from holiday.


Also tangentially, I noticed in machine_pin_cyw43.c that the opening check assumes that if the network is defined and external pins are set, then those pins come from the cyw43. As micropython does not yet support multiple external pin types (unless things have moved on in the last 9 months?), I believe the following line:

#if defined(MICROPY_PY_NETWORK_CYW43) && defined(MICROPY_HW_PIN_EXT_COUNT)

should be changed to:

#if CYW43_GPIO == 1 && defined(MICROPY_PY_NETWORK_CYW43) && defined(MICROPY_HW_PIN_EXT_COUNT)

to support any future Pico W-based Yukon I may attempt.

Would that be something I could include in this PR, or am I better to raise it as a separate issue / PR?

Signed-off-by: Christopher Parrott <chris@pimoroni.com>
Signed-off-by: Christopher Parrott <chris@pimoroni.com>
Copy link
Contributor

@projectgus projectgus left a comment

Choose a reason for hiding this comment

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

This looks good to me, thanks for updating and I hope it makes maintenance easier for the Yukon and other boards!

@projectgus
Copy link
Contributor

Would that be something I could include in this PR, or am I better to raise it as a separate issue / PR?

This machine_pin_cyw43.c change looks very reasonable to me, please free to submit it in a separate PR.

@projectgus projectgus requested a review from dpgeorge September 11, 2024 07:25
@dpgeorge
Copy link
Member

This looks good. Thanks for making it more consistent with the stm32 port.

@dpgeorge
Copy link
Member

Rebased and merged in 5dfd3ec and 79ba6d8

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

Successfully merging this pull request may close these issues.

4 participants