Add support for RP2040 GPIN/GPOUT (clkio) #9009

tinylabs · 2024-03-05T22:39:54Z

Tested on raspberry pi pico.
Add feature to boards/xx/mpconfigboard.mk
CIRCUITPY_RP2CLOCK = 1

Example usage:
import board,rp2clock
clkout = rp2clock.OutputPin(board.GP21, rp2clock.AuxSrc.SYS, 62.5)
// Output on GP21 will now be 2MHz from 125MHz clock SYS clock.

import board,rp2clock
clkin = rp2clock.InputPin(board.GP22, rp2clock.Index.RTC, 1000000, 10000)
// RTC will now be 10kHz from 1MHz input clock on GP22.

Suggestions welcome.

tannewt

Thanks for this change! I haven't looked in detail yet but have some thoughts and questions.

Generally I'd expand clkio to clockio because clk isn't as clear as clock.
What is the difference between auxsrc and index?
Could this be a shared API for clock inputs and outputs where the clock names are defined by a micro-specific module? bindings/samd also does some clock stuff.
What are you using this for?

tinylabs · 2024-03-06T23:56:39Z

Thanks for taking a look @tannewt.

clockio seems like a reasonable substitution.
That is a good question. I admittedly just wrote this as a small wrapper around the rp2040 sdk without digging in further. There is a lot of overlap between the two but they're not identical. Let me dig in some more and see if they can be combined somehow.
ports/atmel-samd/bindings/samd/Clock.c seems to be a ROM representation of the internal clock tree. It doesn't seem to be doing any IO functions so I wouldn't say there is much overlap with this code. As a whole it would be nice to have a unifying API to deal with similar functionality on other micros as I've seen internal clocks available on certain pins in other architectures. That would be a larger undertaking than I'm willing to take on.
I'm using this for a custom PCB which needs a 2MHz clock driven to an external mixed signal device.

tannewt · 2024-03-07T18:02:08Z

I agree standardizing it is a bigger task than need be. Let's call this rp2clockio just to ensure we leave clockio for a unified API. You can remove the Clk prefixes from the class names then too.

tinylabs · 2024-03-07T20:43:18Z

Sounds good, I'll make those changes.

tinylabs · 2024-03-21T16:39:11Z

What do you think about rp2clock instead of rp2clockio? There are other clock related functions in the SDK that may be useful to integrate in the future such as starting/stopping clocks and measuring the frequency of clocks using the onboard hardware.

tannewt · 2024-03-21T16:40:56Z

What do you think about rp2clock instead of rp2clockio? There are other clock related functions in the SDK that may be useful to integrate in the future such as starting/stopping clocks and measuring the frequency of clocks using the onboard hardware.

Fine with me!

tinylabs · 2024-03-22T18:20:30Z

Made the necessary changes. Let me know if it looks ok.

tannewt

Thanks for moving everything around!

tannewt · 2024-03-25T21:34:36Z

locale/circuitpython.pot

Please try to reuse messages in this file instead of adding new ones. That way there is less to translate and less to store in the firmware.

Tried my best to combine them on latest commits.

Just curious, do all translations end up in every build regardless of the modules compiled in? I just assumed that these would only be pulled in when CIRCUITPY_RP2CLOCK=1. I can make the messages more generic to reuse them but we'd lose some information.

Only translations for enabled modules are included. I assume we'll have RP2CLOCK enabled for all RP2040 boards where it'll fit. Striking the generic vs specific balance can be tricky.

ports/raspberrypi/bindings/rp2clock/AuxSrc.c

ports/raspberrypi/bindings/rp2clock/Index.c

ports/raspberrypi/bindings/rp2clock/InputPin.c

tannewt · 2024-03-25T21:40:29Z

ports/raspberrypi/bindings/rp2clock/InputPin.c

+MP_DEFINE_CONST_FUN_OBJ_KW(rp2clock_inputpin_disable_obj, 1, rp2clock_inputpin_disable);
+
+//|     def set_freq(self, src_freq: int, target_freq: int) -> None:
+//|         """Configures the src and target frequency. Must be set before enable() is called."""


Is this just so you can change an existing InputPin? I think you can just delete this, enable and disable and just rely on deinit and reconstruct.

I think you can remove enable(), disable() and enabled too. Instead, folks can just use the constructor and deinit().

Fixed on latest commit.

There are use cases where you'd want to create an external pin object and then enable/disable the clock at runtime. For instance when putting the external chip in low power mode you shouldn't drive the clock to it. My use case actually requires this. This is compounded by the current issue where resources aren't released until garbage collection is called. My opinion is the functionality is required.

.deinit() is the way to turn it off immediately instead of waiting for the finalizer. Do you want to keep the pin "in use" even when it isn't active?

Good point on .deinit(). Since it's a dedicated hardware pin, I think logically it should be use whether the clock is enabled or not. It's akin to driving a normal GPIO, you wouldn't want to release the object and create a new one just to change the value from low to high. The only difference is a high level puts out a clock.

Ok, then you can leave the .enabled property but make it writable instead of having enable() and disable() functions. I'll mark the spot with a pointer to an example.

Sounds good

ports/raspberrypi/bindings/rp2clock/OutputPin.c

ports/raspberrypi/common-hal/rp2clock/InputPin.c

tannewt

Thanks for continuing to refine this! It is getting better and better.

You can test the docs locally with make html from the root circuitpython directory. I suspect you are missing ... function bodies that are used in Python stub files as placeholders.

One comment on enabled and friends and I'd like to hear your thoughts about reducing the number of new error messages. They take up firmware space for everyone.

tannewt · 2024-04-03T20:44:02Z

ports/raspberrypi/bindings/rp2clock/AuxSrc.c

+//|     def __init__(self) -> AuxSrc:
+//|         """Enum-like class to define the clock src."""
+//|     PLL_SYS: "AuxSrc Clock"
+//|     """Undivided PLL used to derive SYS clock."""


tannewt · 2024-04-03T20:44:42Z

ports/raspberrypi/bindings/rp2clock/InputPin.c

+//|         target_freq: Desired frequency for index.
+//|         """
+
+STATIC mp_obj_t rp2clock_inputpin_make_new(const mp_obj_type_t *type, size_t n_args,


Suggested change

//| target_freq: Desired frequency for index.

//| """

STATIC mp_obj_t rp2clock_inputpin_make_new(const mp_obj_type_t *type, size_t n_args,

//| target_freq: Desired frequency for index.

//| """

//| ...

//|

STATIC mp_obj_t rp2clock_inputpin_make_new(const mp_obj_type_t *type, size_t n_args,

tannewt · 2024-04-03T20:48:36Z

ports/raspberrypi/bindings/rp2clock/InputPin.c

+MP_DEFINE_CONST_FUN_OBJ_KW(rp2clock_inputpin_disable_obj, 1, rp2clock_inputpin_disable);
+
+//|     def set_freq(self, src_freq: int, target_freq: int) -> None:
+//|         """Configures the src and target frequency. Must be set before enable() is called."""


I think you can remove enable(), disable() and enabled too. Instead, folks can just use the constructor and deinit().

tannewt

Thanks for the responses. I've made more specific suggestions.

tannewt · 2024-04-05T17:08:43Z

ports/raspberrypi/bindings/rp2clock/InputPin.c

+    return mp_obj_new_bool(self->enabled);
+}
+MP_DEFINE_CONST_FUN_OBJ_1(rp2clock_inputpin_get_enabled_obj, rp2clock_inputpin_get_enabled);
+MP_PROPERTY_GETTER(rp2clock_inputpin_enabled_obj,


Make this settable instead of having enable() and disable().

circuitpython/shared-bindings/digitalio/DigitalInOut.c

Lines 236 to 260 in 8027efe

//| value: bool

//| """The digital logic level of the pin."""

STATIC mp_obj_t digitalio_digitalinout_obj_get_value(mp_obj_t self_in) {

digitalio_digitalinout_obj_t *self = MP_OBJ_TO_PTR(self_in);

check_for_deinit(self);

bool value = common_hal_digitalio_digitalinout_get_value(self);

return mp_obj_new_bool(value);

}

MP_DEFINE_CONST_FUN_OBJ_1(digitalio_digitalinout_get_value_obj, digitalio_digitalinout_obj_get_value);

STATIC mp_obj_t digitalio_digitalinout_obj_set_value(mp_obj_t self_in, mp_obj_t value) {

digitalio_digitalinout_obj_t *self = MP_OBJ_TO_PTR(self_in);

check_for_deinit(self);

if (common_hal_digitalio_digitalinout_get_direction(self) == DIRECTION_INPUT) {

mp_raise_AttributeError(MP_ERROR_TEXT("Cannot set value when direction is input."));

return mp_const_none;

}

common_hal_digitalio_digitalinout_set_value(self, mp_obj_is_true(value));

return mp_const_none;

}

MP_DEFINE_CONST_FUN_OBJ_2(digitalio_digitalinout_set_value_obj, digitalio_digitalinout_obj_set_value);

MP_PROPERTY_GETSET(digitalio_digitalinout_value_obj,

(mp_obj_t)&digitalio_digitalinout_get_value_obj,

(mp_obj_t)&digitalio_digitalinout_set_value_obj);

tannewt · 2024-04-05T17:09:14Z

ports/raspberrypi/bindings/rp2clock/OutputPin.c

+}
+MP_DEFINE_CONST_FUN_OBJ_1(rp2clock_outputpin_get_enabled_obj, rp2clock_outputpin_get_enabled);
+MP_PROPERTY_GETTER(rp2clock_outputpin_enabled_obj,
+    (mp_obj_t)&rp2clock_outputpin_get_enabled_obj);


tannewt · 2024-04-05T17:11:30Z

ports/raspberrypi/common-hal/rp2clock/OutputPin.c

+void common_hal_rp2clock_outputpin_validate_src_pin(const mcu_pin_obj_t *pin) {
+    if ((pin->number != 21) && (pin->number != 23) &&
+        (pin->number != 24) && (pin->number != 25)) {
+        mp_raise_ValueError_varg(MP_ERROR_TEXT("Pin %d invalid, valid pins are: 21,23,24,25"), pin->number);


I'd suggest just using

circuitpython/shared-bindings/microcontroller/Pin.h

Line 45 in 8027efe

NORETURN void raise_ValueError_invalid_pin(void);

because the pin names will vary per-board and the pin number may not be obvious.

tannewt · 2024-04-05T17:11:58Z

ports/raspberrypi/common-hal/rp2clock/InputPin.c

+// GPIN must be [20,22]
+void common_hal_rp2clock_inputpin_validate_index_pin(const mcu_pin_obj_t *pin) {
+    if ((pin->number != 20) && (pin->number != 22)) {
+        mp_raise_ValueError_varg(MP_ERROR_TEXT("Pin %d invalid, valid pins are: 20,22"), pin->number);


I'd just use

circuitpython/shared-bindings/microcontroller/Pin.h

Line 45 in 8027efe

NORETURN void raise_ValueError_invalid_pin(void);

because the pin names will vary by board and the pin number may not be obvious.

tannewt · 2024-04-05T17:14:58Z

ports/raspberrypi/common-hal/rp2clock/InputPin.c

+    if (src == 0) {
+        mp_raise_ValueError_varg(MP_ERROR_TEXT("Invalid freq: %u"), src);
+    }
+    if (target == 0) {
+        mp_raise_ValueError_varg(MP_ERROR_TEXT("Invalid freq: %u"), target);
+    }
+    if (target > src) {
+        mp_raise_ValueError_varg(MP_ERROR_TEXT("Invalid freqs: %u > %u"), target, src);
+    }


I think you can switch these to these validators:

circuitpython/py/argcheck.c

Lines 180 to 206 in 8027efe

mp_int_t mp_arg_validate_int(mp_int_t i, mp_int_t required_i, qstr arg_name) {

if (i != required_i) {

mp_raise_ValueError_varg(MP_ERROR_TEXT("%q must be %d"), arg_name, required_i);

}

return i;

}

mp_int_t mp_arg_validate_int_min(mp_int_t i, mp_int_t min, qstr arg_name) {

if (i < min) {

mp_raise_ValueError_varg(MP_ERROR_TEXT("%q must be >= %d"), arg_name, min);

}

return i;

}

mp_int_t mp_arg_validate_int_max(mp_int_t i, mp_int_t max, qstr arg_name) {

if (i > max) {

mp_raise_ValueError_varg(MP_ERROR_TEXT("%q must be <= %d"), arg_name, max);

}

return i;

}

mp_int_t mp_arg_validate_int_range(mp_int_t i, mp_int_t min, mp_int_t max, qstr arg_name) {

if (i < min || i > max) {

mp_raise_ValueError_varg(MP_ERROR_TEXT("%q must be %d-%d"), arg_name, min, max);

}

return i;

}

You can give the arguments MP_QSTR_src for example for which variable is wrong.

tinylabs · 2024-04-26T16:07:47Z

Sorry for the delay. I went on vacation and have been putting out fires since I got back. Hopefully I can clean this up soon.

tinylabs added 3 commits March 5, 2024 15:31

Add support for RP2040 GPIN/GPOUT (clkio)

d2b1b74

Run make translate, remove whitespace

3b1a31a

Fix code formatting

2f31860

tannewt reviewed Mar 6, 2024

View reviewed changes

tinylabs added 3 commits March 22, 2024 10:27

Rename clkio to rp2clock

128b8c4

Update class names

c60fa3d

Update formatting

a5dd74c

Rebuild translations

ee36308

tannewt requested changes Mar 25, 2024

View reviewed changes

Move to enum macro, make requested changes

bb6a09e

tannewt requested changes Apr 3, 2024

View reviewed changes

tinylabs added 3 commits April 3, 2024 15:48

Fix docs

670df5c

Update docs

d35cda7

Fix doc formatting

852814b

tannewt requested changes Apr 5, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for RP2040 GPIN/GPOUT (clkio) #9009

Add support for RP2040 GPIN/GPOUT (clkio) #9009

tinylabs commented Mar 5, 2024 •

edited

tannewt left a comment

tinylabs commented Mar 6, 2024

tannewt commented Mar 7, 2024

tinylabs commented Mar 7, 2024

tinylabs commented Mar 21, 2024

tannewt commented Mar 21, 2024

tinylabs commented Mar 22, 2024

tannewt left a comment

tannewt Mar 25, 2024

tinylabs Apr 4, 2024

tinylabs Apr 5, 2024

tannewt Apr 5, 2024

tannewt Mar 25, 2024

tannewt Apr 3, 2024

tinylabs Apr 4, 2024

tinylabs Apr 4, 2024

tannewt Apr 4, 2024

tinylabs Apr 5, 2024

tannewt Apr 5, 2024

tinylabs Apr 6, 2024

tannewt left a comment

tannewt Apr 3, 2024

tannewt Apr 3, 2024

tannewt Apr 3, 2024

tannewt left a comment

tannewt Apr 5, 2024

tannewt Apr 5, 2024

tannewt Apr 5, 2024

tannewt Apr 5, 2024

tannewt Apr 5, 2024

tinylabs commented Apr 26, 2024

	//\| value: bool
	//\| """The digital logic level of the pin."""
	STATIC mp_obj_t digitalio_digitalinout_obj_get_value(mp_obj_t self_in) {
	digitalio_digitalinout_obj_t *self = MP_OBJ_TO_PTR(self_in);
	check_for_deinit(self);
	bool value = common_hal_digitalio_digitalinout_get_value(self);
	return mp_obj_new_bool(value);
	}
	MP_DEFINE_CONST_FUN_OBJ_1(digitalio_digitalinout_get_value_obj, digitalio_digitalinout_obj_get_value);

	STATIC mp_obj_t digitalio_digitalinout_obj_set_value(mp_obj_t self_in, mp_obj_t value) {
	digitalio_digitalinout_obj_t *self = MP_OBJ_TO_PTR(self_in);
	check_for_deinit(self);
	if (common_hal_digitalio_digitalinout_get_direction(self) == DIRECTION_INPUT) {
	mp_raise_AttributeError(MP_ERROR_TEXT("Cannot set value when direction is input."));
	return mp_const_none;
	}
	common_hal_digitalio_digitalinout_set_value(self, mp_obj_is_true(value));
	return mp_const_none;
	}
	MP_DEFINE_CONST_FUN_OBJ_2(digitalio_digitalinout_set_value_obj, digitalio_digitalinout_obj_set_value);

	MP_PROPERTY_GETSET(digitalio_digitalinout_value_obj,
	(mp_obj_t)&digitalio_digitalinout_get_value_obj,
	(mp_obj_t)&digitalio_digitalinout_set_value_obj);

	mp_int_t mp_arg_validate_int(mp_int_t i, mp_int_t required_i, qstr arg_name) {
	if (i != required_i) {
	mp_raise_ValueError_varg(MP_ERROR_TEXT("%q must be %d"), arg_name, required_i);
	}
	return i;
	}

	mp_int_t mp_arg_validate_int_min(mp_int_t i, mp_int_t min, qstr arg_name) {
	if (i < min) {
	mp_raise_ValueError_varg(MP_ERROR_TEXT("%q must be >= %d"), arg_name, min);
	}
	return i;
	}

	mp_int_t mp_arg_validate_int_max(mp_int_t i, mp_int_t max, qstr arg_name) {
	if (i > max) {
	mp_raise_ValueError_varg(MP_ERROR_TEXT("%q must be <= %d"), arg_name, max);
	}
	return i;
	}

	mp_int_t mp_arg_validate_int_range(mp_int_t i, mp_int_t min, mp_int_t max, qstr arg_name) {
	if (i < min \|\| i > max) {
	mp_raise_ValueError_varg(MP_ERROR_TEXT("%q must be %d-%d"), arg_name, min, max);
	}
	return i;
	}

Add support for RP2040 GPIN/GPOUT (clkio) #9009

Are you sure you want to change the base?

Add support for RP2040 GPIN/GPOUT (clkio) #9009

Conversation

tinylabs commented Mar 5, 2024 • edited

tannewt left a comment

Choose a reason for hiding this comment

tinylabs commented Mar 6, 2024

tannewt commented Mar 7, 2024

tinylabs commented Mar 7, 2024

tinylabs commented Mar 21, 2024

tannewt commented Mar 21, 2024

tinylabs commented Mar 22, 2024

tannewt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tannewt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tannewt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tinylabs commented Apr 26, 2024

tinylabs commented Mar 5, 2024 •

edited