ra-rs / ra

Rust PACs for the Renesas RA series devices
Apache License 2.0
8 stars 4 forks source link

Added a peripheral Patch for the RA4M1 chip. #1

Open oshaughnessya opened 1 year ago

oshaughnessya commented 1 year ago

I added a GPIO peripheral Patch as I didn't find the current SVD quite usable for GPIO.

The patch gets rid of the PCNTR[1-4] registers which leaves only the 16-bit registers which make up those 4 32-bit registers. I could possibly change it to get rid of the 16 bit registers and keep the 32 bit registers, but this felt more right to me.

I also changed the fields to be of the individual bits instead of the whole field, as that makes it more ergonomic while programming using the PACs, this is also how ST organizes their SVDs.

oshaughnessya commented 1 year ago

@n8tlarsen it seems you are the maintainer of this, but I don't know if it sent you a notification since the repo is under ra-rs instead of you

n8tlarsen commented 1 year ago

Hi, sorry I didn't get a notification for your PR. Thanks for contributing! I'll take a look at it shortly 🙂

n8tlarsen commented 1 year ago
  1. I'd like to set up some Github actions to run the update script before pulling in your changes. I think this just helps everyone be confident in the robustness of the software, and makes less work for me in the long run lol.
  2. I like the idea of splitting the fields into individual bits for the purpose of using the PAC directly without a HAL. I think a well-written HAL would likely use some bit-shifts to make the GPIO peripheral access generic. Since there's currently no HAL for this PAC this is a good quality of life improvement.
  3. I think there's value in keeping the 32-bit register access since it provides the compiler with more opportunities for optimizations. While both 32-bit and 16-bit access is possible for these registers, each operation takes one cycle. So defining only 16-bit access could double the cycles in some I/O-intensive applications.
  4. For clarity, I'd like to see the name of the file match the name of the peripheral being patched. I think it's important to match datasheet nomenclature whenever practical.
  5. I realize you're probably only working with the RA4M1 so other families probably aren't as important to you, but it would be nice to keep the same API for other RA families implementing these registers, which is likely most or all of them. My first point should help with testing this out.
oshaughnessya commented 1 year ago
oshaughnessya commented 1 year ago

Struggling with github over here, sorry for the close.

  • I like the idea of splitting the fields into individual bits for the purpose of using the PAC directly without a HAL. I think a well-written HAL would likely use some bit-shifts to make the GPIO peripheral access generic. Since there's currently no HAL for this PAC this is a good quality of life improvement.

I do intend on trying to make the HAL after I get some more peripherals patched in, I will have to see how others are made, but doing it this way I should still be able to set the individual bits without issue due to how svd2rust works

  • I think there's value in keeping the 32-bit register access since it provides the compiler with more opportunities for optimizations. While both 32-bit and 16-bit access is possible for these registers, each operation takes one cycle. So defining only 16-bit access could double the cycles in some I/O-intensive applications.

I'll try to get that changed over this weekend if i can find time

  • For clarity, I'd like to see the name of the file match the name of the peripheral being patched. I think it's important to match datasheet nomenclature whenever practical.

I was thinking about that, but I didn't see the exact name of the peripheral when I was looking at the datasheet, I'll take a closer look.

  • I realize you're probably only working with the RA4M1 so other families probably aren't as important to you, but it would be nice to keep the same API for other RA families implementing these registers, which is likely most or all of them. My first point should help with testing this out.

I was thinking about that one too, but I didn't feel like going over the SVDs and datasheets of the rest of the chips to make sure that it worked properly

oshaughnessya commented 1 year ago

3. I think there's value in keeping the 32-bit register access since it provides the compiler with more opportunities for optimizations. While both 32-bit and 16-bit access is possible for these registers, each operation takes one cycle. So defining only 16-bit access could double the cycles in some I/O-intensive applications.

Thinking about this one a bit more, specifically the svd2rust output, I feel like it would be easier for writing code where you are interacting with the raw register values to not have to think about if you need to shift to the upper half or the lower half of the 32bit register.

and when it comes to having high I/O intensive programs, most of these registers are for setup, the only register I can really see where you may want to interact with both halves is PCNTR1 since it has both pin direction and the output data registers

elfmimi commented 1 year ago

Hi. I have that impression that fine grain GPIO access is to be done by help of a HAL, which we don't have yet.