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

(ESP32 only) Support for the high-speed channels #424

Merged
merged 2 commits into from
May 26, 2024
Merged

Conversation

ivmarkov
Copy link
Collaborator

To be precise, it is not exactly the case that high-speed channels are not currently supported.

The problem is in how they are supported: currently, there are only 8 exposed channel peripherals and 4 exposed timer peripherals.

However, the ESP32 actually has two groups of LEDC channels + drivers:

  • High-speed group - 8 channels, 4 timers
  • Low speed group - 8 channels, 4 timers (the only types all other chips have)

So in total the peripherals are: 16 channels, 8 timers. Since the two groups of channels and timers are currently overlaid over a single set of channels + timers, user has to choose - at driver construction time - whether she wants to treat channel X and/or timer Y as low speed or high speed.

However, this effectively limits the total number of channels and timers that can be instantiated to 8 + 4, instead of 16 + 8.

In other words, I can't have 3 RGB diodes running on the Esp32, as that would require 12 active channels, while currently I can only have 8 active channels. :)

=============

This PR introduces separate, additional channels and timers for the high speed peripherals available on the esp32 (a new peripherals.hledc member). The existing peripherals.ledc are now hard-coded to always represent the low-speed ones.

The PR is a breaking change in that:

  • Users which are in need of high speed channels / timers, now explicitly have to use the HLEDC group
  • The speed_mode timer config is gone, as it is now a property of the timer peripheral
  • The LedcTimer driver (unfortunately) is now generified by the timer peripheral, as we need to obey the invariant, that we can only pass a high speed timer to a high speed channel and the other way around, but can't mix. The current API does not enforce this at compile time, but also doesn't have to - as the timer itself dictates what channel you'll get - high speed for a high speed timer, and low speed for a low speed timer.

Self::LowSpeed
}
/// Low speed mode for the LED Control peripheral
pub struct LowSpeed;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One nit: This would mark every other LEDC peripheral on every other esp variant as only having a "LowSpeed" variant. But one could argue that its neither low nor highspeed on every other esp. So for all users expect the esp32 its a bit wired to have a LowSpeed variant, as it would imply there is another one on the used chip itself.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But i see when using this on a esp32 that this naming scheme make sense.

Copy link
Collaborator Author

@ivmarkov ivmarkov May 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You read my mind. I had the same annoyance, but not sure how to fix, other than renaming LowSpeed to - say - RegularSpeed.

The current driver version also names it LowSpeed in the SpeedMode enum I just retired.

So what shall it be? Leave as-is or rename to RegularSpeed?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, i also have no super good idea how to handle it concisely. The root cause is that we would not be generic over the LEDC Timer Driver with the extra generic term, but we would need to model as it is in reality. E.g we have two different peripheral types with "the same name". We would need to be generic over the TimerConfig. But i see that this is cumbersome since we would need two different proc-macros for impl_timer! one that is only for the esp32 and one for the rest. That way we can give them each different names.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So yeah i don't want to block on it, either its more code if you up to it, or let it be

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand what you are suggesting. Would you please elaborate a bit?

In any case, having a single driver type for a channel (LedcDriver) and a single driver type for a timer (LedcTimer - even if parameterized by the peripheral type) is IMO an advantage, not a disadvantage, so ideally we should keep it that way. If that's what you suggest to break.

@Vollbrecht Vollbrecht merged commit 5a741ae into master May 26, 2024
8 checks passed
@Vollbrecht
Copy link
Collaborator

LGTM! Thanks

@ivmarkov ivmarkov mentioned this pull request Jun 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants