Initial ESP32-C3 GPIO HAL driver

[DNedic]

This adds a first iteration of the ESP32-C3 (later to be universal) GPIO HAL driver, and factors it out into its own file.

[DNedic]

I will be changing the approach to the API, there is simply too much variation in possible configurations to have everything as a typestate, and having Output and Input separately when there will still be runtime differences is a half-measure.

[DNedic]

Thanks for the review @haydenridd !

[DNedic]

Thanks for reviews @haydenridd @ikskuh !

Could you explain what benefits the approach taken in the Raspberry Pi Pico HAL provides in this case?

[haydenridd]

Thanks for reviews @haydenridd @ikskuh !

Could you explain what benefits the approach taken in the Raspberry Pi Pico HAL provides in this case?

For sure! Also just for clarity, the example I gave was general to any peripheral, not just a "Pin". The benefits I see are as follows:

• Given there will be a variety of types/functions name-spaced under hal.i2c, for example I2C, TransactionError, TimingRegisterValues, etc, it's nice that there is a standardized namespace within hal.i2c to "get a given instance of a peripheral"
  • No matter the peripheral, there is a consistent experience of accessing said peripheral via hal.peripheral.instance
• In the general case, a user would access a peripheral via hal.i2c.instance.I2C0/1
• If for some reason a user needs to programatically access the instance via number, that's where hal.i2c.instance.num(0) comes in. Use cases for this will be fewer, but adds convenience if for instance you wanted to configure all instances in a loop.
• Finally, if there is a peripheral where there is only ever a single instance (ADC on the RP2040), its methods would just be directly accessed via hal.i2c.instance.method()

So, essentially, this approach is just for consistency and convenience. It also attempts to provide some intuition that peripherals are not something that can be created or destroyed, but rather are inherent global entities that preserve state throughout program execution.

[DNedic]

Thanks for the clarification @haydenridd , this makes sense to me. I have some conflicts of taste with the approach, though nothing deal-breaking.

• Even though we cannot 'create' peripherals, we can copy the instance to name it, like so: led_pin = gpio.instance.GPIO8, multiple times as well, so in a sense this does not seem too different from led_pin = gpio.Pin.init(8)
• Besides this, by using init(), we are ensuring that the peripheral is always initialized before attempting to use it, while using apply to initialize in the other case can be easily forgotten
• (nitpick) The final convenience feature, single instance case hal.i2c.instance.method() is a bit of an outlier and might confuse people if they are used to using instance.instance, can we have global instances without the instance namespace and save some typing?
• (nitpick, does not conflict with the approach mentioned in a fundamental way) Using enums as underlying types and then @enumfromint() is less verbose while I feel it is more confusing to newcomers and less readable overall than using single-member structures

@ikskuh cc

[haydenridd]

To address your points:

• Even though we cannot 'create' peripherals, we can copy the instance to name it, like so: led_pin = gpio.instance.GPIO8, multiple times as well, so in a sense this does not seem too different from led_pin = gpio.Pin.init(8)
• Besides this, by using init(), we are ensuring that the peripheral is always initialized before attempting to use it, while using apply to initialize in the other case can be easily forgotten

Yup, for sure, I think as you say this just comes down to personal taste. Basically my:

const led_pin = gpio.Pin.instance.num(8);
led_pin.apply(...);

Equals your:

const led_pin = gpio.Pin.init(8);

The only thing I'll caution is if init() configures the peripheral and passes you an instance, does it handle this peripheral being previously configured? As in, does it know/care about this sort of behavior:

const led_pin1 = gpio.Pin.init(8);
led_pin1.write(...)...
// Does this "reset" to a known state before performing initialization?
// Is using the variable led_pin1 still valid?
const led_pin2 = gpio.Pin.init(8); 

• (nitpick) The final convenience feature, single instance case hal.i2c.instance.method() is a bit of an outlier and might confuse people if they are used to using instance.instance, can we have global instances without the instance namespace and save some typing?

This is a fair point. I could honestly go either way.

• (nitpick, does not conflict with the approach mentioned in a fundamental way) Using enums as underlying types and then @enumfromint() is less verbose while I feel it is more confusing to newcomers and less readable overall than using single-member structures

Yup this one is trivial enough I don't have a super strong opinion on it either way. No problem with your method here!

[DNedic]

The only thing I'll caution is if init() configures the peripheral and passes you an instance, does it handle this peripheral being previously configured? As in, does it know/care about this sort of behavior:

Good point there, a check could be added to peripherals where double initialization would for some reason cause problems by either adding an initialized: bool member, or infering from registers.

Generally, I would let users track this and would not attempt to protect them from a case like this, as the solutions can get unwieldly very quickly. The global instance situation has the same problem, so there is really no difference when it comes to that in the approaches, the only difference is that you can't have an uninitialized peripheral unless you manually deinitialize/reset.

A counter question would be, apply(config) has configure semantics, while init() has configure + enable in cases where there is no enable in config in my opinion (very subjective), so would a separate enable() function be considered for apply()?

I'm going to adapt my solution to the WIP guidelines we have nevertheless in the meantime and we can merge the changes, however these are some things I'd like to see evaluated further.

[DNedic]

@haydenridd PTAL again

[DNedic]

Dilemma: Should I use the functions for setting individual options in the constructor to make it more verbose?

Whether this makes code easier to read is up to discussion (I am leaning towards yes), but there is the issue of register writes being volatile, and with that approach more write operations would be created by the compiler, bloating code size. In GPIO this might not be terrible but I imagine for more complex peripherals this can add up.

[haydenridd]

A counter question would be, apply(config) has configure semantics, while init() has configure + enable in cases where there is no enable in config in my opinion (very subjective), so would a separate enable() function be considered for apply()?

A good question, I would say it's a little case dependent. "Enabling" the peripheral can in many cases do "nothing" other than make it ready for use. For instance, enabling the I2C block, but understanding that nothing will happen until you call some sort of read or write function. In that case it would make sense to enable in apply. I agree it's a bit subjective and case by case!

[haydenridd]

Dilemma: Should I use the functions for setting individual options in the constructor to make it more verbose?

Whether this makes code easier to read is up to discussion (I am leaning towards yes), but there is the issue of register writes being volatile, and with that approach more write operations would be created by the compiler, bloating code size. In GPIO this might not be terrible but I imagine for more complex peripherals this can add up.

I think you can have the best of both worlds if you mark those configuration functions that just touch a register inline... Then it would be the "same" behavior whether you use those functions or not in apply

[DNedic]

I think you can have the best of both worlds if you mark those configuration functions that just touch a register inline... Then it would be the "same" behavior whether you use those functions or not in apply

Are you sure this would merge the volatile writes? In C or C++ this would not happen even if they were in the same function, line after line.

[haydenridd]

Are you sure this would merge the volatile writes? In C or C++ this would not happen even if they were in the same function, line after line.

Great question... I think so. inline works quite differently in Zig than in C/C++, it's a mandate to the compiler rather than a suggestion. Maybe I'm misinterpreting what you mean by "merge" though!

EDIT: I think I get what you mean now. Nope, the compiler will not combine multiple writes to the same register into a single write due to it being volatile, so you are correct hard coding is the way to go here!