Skip to content

zephyr: Construct Pin object with a port instance number. #10859

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

Conversation

MaureenHelm
Copy link
Contributor

Adds support for constructing a Pin object with a port instance number, where the available instances are defined by devicetree chosen nodes. Existing support for constructing a Pin object with a device name string remains, but device names will become less user-friendly when upgrading to Zephyr v3.2.0 due to deprecation of the devicetree label property.

Adds support for constructing a Pin object with a port instance number,
where the available instances are defined by devicetree chosen nodes.
Existing support for constructing a Pin object with a device name string
remains, but device names will become less user-friendly when upgrading
to Zephyr v3.2.0 due to deprecation of the devicetree label property.

Signed-off-by: Maureen Helm <maureen.helm@intel.com>
@dpgeorge
Copy link
Member

Thanks for the patch.

It looks like this will require a overlay (for the device tree) for every board? And then every peripheral within that board will need to be listed (GPIO, SPI, UART, etc), is that correct?

Is it possible to provide a generic overlay for a particular MCU series that will work on all boards using that MCU? (Or even better a single, generic overlay for everything?)

@bogdanm
Copy link

bogdanm commented Feb 27, 2023

Take a look at the solution proposed in #10830, it should be a bit more generic I think.

@jimmo
Copy link
Member

jimmo commented Feb 28, 2023

@MaureenHelm @bogdanm -- Thanks both of you for looking at this! It has been on my TODO list, but I am relatively new to Zephyr and device tree and haven't had the time to really try and understand this.

I have looked at this PR and #10830 (and of course the original #9335 to move to v3.2.0).

From MicroPython's perspective, we'd like to see three things:

  • We really don't want to have to maintain board configurations/overlays. The goal is for our port to be able to build for any supported Zephyr board and get at least the basic functionality.
  • It should work as closely as possible to other ports in terms of the expectations we place on the user -- i.e. it should not be a requirement that they know what the base address for a peripheral is.
  • For someone coming to MicroPython from Zephyr, the experience should be similar (i.e. things should be named the same).

We (@dpgeorge and I) are very keen to do more with the Zephyr port, in particular to support the nRF family, to the extent that I'd like to remove our nRF port in favour of making the Zephyr port feature-parity. I don't particularly mind about (once-off) breaking backwards compatibility on the Zephyr port in order to achieve any of the above.

So... first up... I'm curious why Zephyr made the change here. Why was the devicetree label property deprecated? It seems useful? What was the downside?

I'll address the two PRs together here, mostly because my comments are fairly high-level...

  1. @MaureenHelm As @dpgeorge asked above, it seems like this PR will require us to maintain an overlay for every possible board. I don't think this is going to work for us. Or are we misunderstanding?
  2. @bogdanm In ports/zephyr: Update to Zephyr 3.2.0. #10830 your script looks like it avoids this problem, but I'm not sure how this will work across all boards? For example, on the nRF5340_DK, it uses gpio0, gpio1, etc, which get mapped by your script to "GPIO_0", "GPIO_1", etc. But the K64F board in this PR uses gpioa, gpiob, etc which your script will not match. I imagine the script can be made more sophisticated, but I don't have a feel for how much diversity of configurations there is?
  3. Instead of mapping them to MicroPython names, could we instead keep their names. i.e. If I use MicroPython on a K64F, then it's machine.Pin(("gpioa", 5), ...) and on a nRF53 it's machine.Pin(("gpio0", 5), ...). (This is kind of my point 3 above). Is there a way to adapt your script to just enumerate all things of a particular type, rather than trying to make everything fit into "GPIO_N"?)

Thanks!

@bogdanm
Copy link

bogdanm commented Feb 28, 2023

Hi @jimmo, thank for your reply. The reason for the change is explained in some detail in https://docs.zephyrproject.org/3.2.0/releases/release-notes-3.2.html#devicetree. While I am not a Zephyr expert, I think I understand their reasoning. Their macro-based alternative (DEVICE_DT_GET(DT_NODELABEL(lbl))) works for the vast majority of real world use cases out there, but Zephyr is a fairly particular use case, since it requires instantiation of DT nodes at runtime by name.

My script can easily be changed to generate gpio0/gpioa/whatever is found in the DT instead of GPIO_0, but personally I'd like to take a bit more time (a part of today) to figure out a way to make it more generic in its current form. I'm fairly sure that I can mostly automate a process in which I look at the GPIO/I2C/SPI/UART labels in 3.1.0, group their naming per platform (I expect to find a fairly limited list of choices, there can't be much more than gpio0 or gpioa in there), then add a --platform argument to my script so that it can generate the correct translation to the old label names. In my book, that's a small price to pay to ensure backward compatibility, I just don't want to give up on it without a bit of a fight :) But of course, this is ultimately up to you, so please let me know what you think.

@jimmo
Copy link
Member

jimmo commented Feb 28, 2023

Thanks for the additional context @bogdanm

I guess the guiding principles here should be:

  • The Zephyr port should work like other MicroPython ports.
  • Using MicroPython should also match the experience for Zephyr developers using C. If a C developer would call it gpioa (e.g. by writing DT_NODELABEL(gpioa), then we should match that.

My understanding of https://docs.zephyrproject.org/3.2.0/build/dts/api-usage.html#dt-node-identifiers is that node labels are named for the way the SoC names them. Fortunately this matches what we do in MicroPython for other ports. e.g. on STM32 you refer to pins as "An", "Bn", etc. On rp2, it's "GPIOn", i.e. we match the way the datasheet or vendor SDK refers to them.

Zephyr is the only port where we do the tuple thing for machine.Pin, so it's a bit of an odd one out there, so I'd like to fix that too. It's also one of the few remaining ports where machine.Pin.cpu.PinName isn't implemented.

Should find out what @dpgeorge thinks, but I think what I'd like to see on Zephyr is:

machine.Pin("<nodelabel>_n")
machine.Pin.cpu.<nodelabel>_n
machine.I2C("<nodelabel>")
... etc

(In other words, if we're going to break backwards compatibility we might as well go all the way and drop the tuple syntax for pins and make it exactly like the other ports, right down to the way make-pins.py works on other ports. The only difference is that make-pins.py is using something like the code you've written to query the device tree in order to generate the tables, compared to e.g. the stm32 one that parses the stm hal).

There's a whole separate discussion to be had around how to implement machine.Pin.board.FOO like the other ports. Does devicetree have a way to refer to a single pin, and if so, is this used in Zephyr? Or is this going to have to be done in MicroPython-specific configuration?

@bogdanm
Copy link

bogdanm commented Feb 28, 2023

@jimmo I'm new to MicroPython in general and I only used it with Zephyr, so I'll take your word for it :) Just let me know how I can help to make this happen.

Does devicetree have a way to refer to a single pin, and if so, is this used in Zephyr?

Not in general, as far as I can tell. The code to access a GPIO pin would look something like this:

// First get the port instance
const struct device *port = DEVICE_DT_GET(DT_NODELABEL(gpio0/gpioa/whatever));
// Then configure the pin inside that port (input/output/drive and so on)
gpio_pin_configure(port, pin_number, pin_flags);

There are some things for which you can specify a port/pin combo. For example, you can define LEDs (https://github.com/zephyrproject-rtos/zephyr/blob/main/boards/arm/stm32h735g_disco/stm32h735g_disco.dts#L22-L32) or keys (https://github.com/zephyrproject-rtos/zephyr/blob/main/boards/arm/stm32h735g_disco/stm32h735g_disco.dts#L34-L40), but these are specific DT features. For generic GPIO access you are stuck with the port/pin pair, but there's probably a way to make this happen in MicroPython too. I'm thinking that the C code can parse machine.Pin("<nodelabel>_n") and automatically figure out the port name and pin number from that.

@MaureenHelm
Copy link
Contributor Author

MaureenHelm commented Mar 6, 2023

It looks like this will require a overlay (for the device tree) for every board? And then every peripheral within that board will need to be listed (GPIO, SPI, UART, etc), is that correct?

Yes, that is correct. There isn't a general way to enumerate a list of devices of a given type (i.e., all I2C devices or all UART devices). It's possible for sensors via zephyrproject-rtos/zephyr#51352 but this pattern hasn't been extended to other device types.

Is it possible to provide a generic overlay for a particular MCU series that will work on all boards using that MCU?

Probably, but I'm not sure how much that really helps here.

My understanding of docs.zephyrproject.org/3.2.0/build/dts/api-usage.html#dt-node-identifiers is that node labels are named for the way the SoC names them. Fortunately this matches what we do in MicroPython for other ports. e.g. on STM32 you refer to pins as "An", "Bn", etc. On rp2, it's "GPIOn", i.e. we match the way the datasheet or vendor SDK refers to them.

Right, and unfortunately I think this will make @bogdanm 's script hard to maintain across SoC families.

We (@dpgeorge and I) are very keen to do more with the Zephyr port, in particular to support the nRF family, to the extent that I'd like to remove our nRF port in favour of making the Zephyr port feature-parity.

That's great to hear!

@beriberikix
Copy link

It looks like this will require a overlay (for the device tree) for every board? And then every peripheral within that board will need to be listed (GPIO, SPI, UART, etc), is that correct?

Yes, that is correct. There isn't a general way to enumerate a list of devices of a given type (i.e., all I2C devices or all UART devices). It's possible for sensors via zephyrproject-rtos/zephyr#51382 but this pattern hasn't been extended to other device types.

FWIW, the Arduino GSoC project also had the same desire and looked into automating generating overlay files. We ran out of time last year but it seemed feasible given how simple the overlays need to be (at least for the Arduino Core.) I imagine something similar could apply for MP.

@bogdanm
Copy link

bogdanm commented Mar 7, 2023

Right, and unfortunately I think this will make @bogdanm 's script hard to maintain across SoC families.

I don't think the script would require much maintenance. This is (hopefully) a one time change in Zephyr, so the script would have to be written once, verified that it works correctly and then happily do its job ever after :) I'm not even married to the idea of translating the node names (gpio0 can remain gpio0 (the name in the DT) as opposed to GPIO_0 or even GPIO0). I agree that veryfing that the script works correctly is hard given the large number of platforms that Zephyr supports and the script might have some bugs initially because of this, but I think that the occasional bug fixes to the way the script generates names on some platforms would be the only maintanance required.
The other solution proposed here (automatically generating overlay files) also implies some additional scripting, so it looks like both approaches agree that some more information is needed by the Zephyr port in order to be usable. Personally I am not partial to either of these approaches, I'd just be happy to see a decision about the way forward so that we can make it happen.

@bogdanm
Copy link

bogdanm commented Mar 21, 2023

Any news on this? I appreciate that this kind of thing moves slowly, but think about the benefits: as someone mentioned above, this could potentially replace the nRF port completely (I can personally testify that the nRF support in Zephyr is very good). And who knows, maybe other platforms too.

@dpgeorge
Copy link
Member

Any news on this?

Fixing this is important but we need to fully understand the best way forward. And the pending v1.20.0 release of MicroPython is a higher priority right now.

@yonsch
Copy link
Contributor

yonsch commented Jun 12, 2023

Any news?

@bogdanm
Copy link

bogdanm commented Jun 12, 2023

Sadly, this appears to be a low priority item for the dev team, which is unfortunate because it forces me (and likely others) to work on a fork, which is rarely a good thing. But I still hope for some good news 🤞

@projectgus
Copy link
Contributor

This is an automated heads-up that we've just merged a Pull Request
that removes the STATIC macro from MicroPython's C API.

See #13763

A search suggests this PR might apply the STATIC macro to some C code. If it
does, then next time you rebase the PR (or merge from master) then you should
please replace all the STATIC keywords with static.

Although this is an automated message, feel free to @-reply to me directly if
you have any questions about this.

@dpgeorge
Copy link
Member

@MaureenHelm I think we can close this, because it was superseded by #9335.

Or at the very least, this needs reworking on the latest master.

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

Successfully merging this pull request may close these issues.

7 participants