Skip to content
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

Peripherals that can be split, should have their config split accordingly #2931

Closed
MabezDev opened this issue Jan 10, 2025 · 9 comments · Fixed by #2965
Closed

Peripherals that can be split, should have their config split accordingly #2931

MabezDev opened this issue Jan 10, 2025 · 9 comments · Fixed by #2965
Assignees
Labels
Milestone

Comments

@MabezDev
Copy link
Member

MabezDev commented Jan 10, 2025

See #1668 (comment) for context. I think this only affects Uart, from the stabilized driver list.

With this setup, and the ideas following from that, eventually we can add split support where:

  • The full Uart takes Config
  • The control struct from split takes the full Config
  • UartRx take the RxConfig
  • UartTx takes the TxConfig

The important thing here is that we structure the config in a way that allows us to do something like this post 1.0.

We should also update the API docs accordingly.

@bugadani
Copy link
Contributor

The control struct from split takes the full Config

This part is dangerous. We shouldn't allow configuring parts of the driver that may be in active use.

@playfulFence
Copy link
Contributor

Maybe we should have only runtime-modifiable things in that Control struct?

@bugadani
Copy link
Contributor

Maybe we should have only runtime-modifiable things in that Control struct?

You might want to specify that, as all config options are runtime-modifiable currently.

@playfulFence
Copy link
Contributor

playfulFence commented Jan 14, 2025

Does it make sense to runtime-modify StopBits, Parity or DataBits? Maybe it does though, not saying it doesn't 😄
Maybe only baudrate and clock source should be in Control structure?

@bugadani
Copy link
Contributor

bugadani commented Jan 14, 2025

Would you say it safe to modify the clock source, or the baud rate, when the peripheral is sending or receiving data? (Yes this is a trick question, UART is always receiving data)

@MabezDev
Copy link
Member Author

MabezDev commented Jan 14, 2025

Would you say it safe to modify the clock source, or the baud rate, when the peripheral is sending or receiving data? (Yes this is a trick question, UART is always receiving data)

As you mentioned, UART is always listening, so this is always something the user will have to handle when changing almost anything related to uart protocol - we can't really help them.

For future peripherals where the protocol isn't always listening, we can add some internal locking to avoid starting/stopping tx/rx halves whilst the protocol is being changed.

This is all a bit orthogonal to the issue at hand here (unless I'm missing the point you're getting at), we're just prepping the configuration layout for the future?

@bugadani
Copy link
Contributor

bugadani commented Jan 14, 2025

As you mentioned, UART is always listening, so this is always something the user will have to handle when changing almost anything related to uart protocol - we can't really help them.

Let me phrase this in another way: a shared configuration object, without locking in the driver, is UB if the split halves have their own partial configuration functions.

The clock source/baud rate thing is somewhat orthogonal, but still related to what we allow to do, and it's a direct response to the question Kirill asked.

I'm pretty much okay with separate config halves. We can handle in the Rx driver that we must work around some things - like we can disconnect the RX pin for the duration of the configuration. But we shouldn't allow some external piece of code to do that, I think.

@MabezDev
Copy link
Member Author

Let me phrase this in another way: a shared configuration object, without locking in the driver, is UB if the split halves have their own partial configuration functions.

I agree, we'd need to have some form of locking/communication to make global uart changes to whilst rx/tx are split.

But we shouldn't allow some external piece of code to do that, I think.

I think I agree, but I also don't think I'm 100% certain what you mean by this. In my head, all this should be internal to the HAL. Once we have a split with tx, rx and control, tx & rx should be able to manage their own config themselves, but control will need some way to tell the tx/rx halves we're about to make global changes prepare for it. As you mentioned this might include disconnecting the pin during the configuration. Is this the sort of thing you meant?

@playfulFence
Copy link
Contributor

playfulFence commented Jan 23, 2025

Currently, this issue is not applicable to any other peripheral driver we plan to stabilize by [email protected], so I'm closing this issue.

@github-project-automation github-project-automation bot moved this from Todo to Done in esp-rs Jan 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants