[RP2xxx] Update pins helper to create pins as comptime-available data instead of zero-sized-types #303
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
MatthiasPortzel
changed the title
mattnite
requested changes
Dec 8, 2024
mattnite
approved these changes
Dec 19, 2024
This refactor maintains one of the very nice properties of the original approach: `pins.led_a` (for example) is comptime known. In order to preserve this property while also having `pins.led_a` be data (i.e. the existing gpio.Pin type) we needed to make pins accessible at comptime—before .apply has been called. This refactor has a couple of parts * Move `Pins(comptime config: GlobalConfiguration) type` to `PinsType(self: GlobalConfiguration) type` * Move the return value from `GlobalConfiguration.apply` to `GlobalConfiguration.pins` (so `.apply` returns `void`. This allows `GlobalConfiguration.pins()` to be called at comptime, which allows sub-values of `pins` to be read at comptime. * Change pins to be data instead of a type--replace Pwm with the new RuntimePwm and replace pins.GPIO with the existing gpio.Pin. The value of this is set in GlobalConfiguration.pins. ADC is left broken, since I believe it's broken on main. Will be fixed in a later commit.
mattnite
force-pushed
the
pins-as-data
branch
from
33fe622 to
edf3034
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
TL;DR: Replaces
pins = pin_config.apply()withpins = comptime pin_config.pins(); pin_config.apply().Long version:
This PR updates the "pin configuration helper,"
pins.GlobalConfiguration, to return apinstype which is more suitable for normal usage than the ad-hoc zero-sized-types previously created. For example, apins.led_acould be saved into a runtime struct or passed to a struct initializer at comptime, just like a primitive Zig value. In order to make this possible, there are a couple of distinct changes being made:gpio.Pin. This removes the previous ZSTs and unifies the API. As well as making these data that can be used at runtime.pin_config.apply()return thepinstype, as was previously the case,.apply()now returns nothing andpin_config.pins()returns thepins. This was necessary in order to allowpinsto exist as normal Zig data. (Sinceapplyhas to be called at runtime, only the type of the return value is known atcomptime.)The only breaking change for users is splitting calls to
apply()into a new call topins(). (See the change to the examples for an example of what that looks like.)This represents a regression in that you will no longer get compile-errors for e.g. reading from an output pin. This is acceptable to me for the time being because it matches our philosophy for the rest of the HAL—configuration is the user's responsibility. However, I'm not opposed to changing the GPIO interface to be stricter, and I have a couple of ideas on how to do that. But fundamentally I think that change would need to happen to
gpio.Pin(e.g. maybe it splits intogpio.ReadPinandgpio.WritePin)—I don't think it's tenable to have a split in the type system between a configured pin and an unconfigured pin.I'm 99% sure that accessing an ADC-type pin on main right now doesn't work. Since the ADC code is old, it could really use a rewrite, so I've left it broken in this PR (replaced it with a TODO). I'm happy to update the pin configuration helper to support ADC if it's clear what we want to do with the HAL.