Add LEDC duty-cycle fading support

[BryanKadzban]

This code may work for other chips as well; I haven't read their reference manuals. For now it's feature-gated to just the c3.

And without interrupts it's hard to tell when a fade has finished (for example to start another), but it seems like a reasonable start to me.

[BryanKadzban]

Fixed rust formatting. Not completely sure what's up with the xtensa run, though. If that fails again I'll keep digging.

[bjoernQ]

Looks like a great start. Would be nice to have examples and look into supporting also other chips as well

[BryanKadzban]

I've gotten a chance to read through the reference manuals for the other chips. Looks like all of them work the same way except the -c6, which has a bunch of gamma curve support in the fading. That's nice (and I wish it was available in the C3 that I'm able to get ahold of), and might be good to support in the future, but I think just setting up a single gamma segment if the C6 feature is present is the right way to go for this. It may be a while before I can get to that but we'll see.

But there's something I'm less sure of. Maybe it makes sense to have this whole API be async from the beginning? I see a lot of complication around feature flags for async-ness in the rest of the HAL; is that because those APIs existed first as a sync variant, and so changing to async needs to be done carefully? Or is there another reason to make this sync from the first time it's available?

I'm thinking of installing a global interrupt handler to set a flag, and using that with async/await (...somehow; I haven't fully figured out how it would work) to let the future know the fade is done. WDYT?

[bjoernQ]

Awesome you took the time to read through the TRMs - would be great to get this supported!

Regarding async: Having async support in addition would be awesome however we should always provide sync and optionally async APIs but never only async APIs since a sync API (to some extend) can be used in async code (by using callbacks, interrupts etc.) but the other way around is much harder and hackier if possible, at all

Another reason why async is feature gated is: if possible, we implement traits from embedded-hal-async but that is in alpha

I totally agree, that supporting sync and async APIs currently require a lot of duplicate code. Not sure if keyword-generics will really solve this in future

I'm thinking of installing a global interrupt handler ...

Maybe I'm misunderstanding but the HAL shouldn't do that "behind the scenes". For now, we should do it like the existing async-drivers (e.g.

esp-hal/esp32c3-hal/examples/embassy_wait.rs

Lines 77 to 82 in e01a569

	// Async requires the GPIO interrupt to wake futures
	esp32c3_hal::interrupt::enable(
	esp32c3_hal::peripherals::Interrupt::GPIO,
	esp32c3_hal::interrupt::Priority::Priority1,
	)
	.unwrap();

) where the interrupt handler is defined in the HAL (but feature gated!) but enabling the interrupt is still the user's responsibility. Probably a convenience function to do that is also a good idea but at least it should be an explicit step (for now)

In that interrupt handler you can directly wake the waker for async (like done here:

esp-hal/esp-hal-common/src/gpio.rs

Line 1942 in e01a569

PIN_WAKERS[pin_nr as usize].wake(); // wake task

)

[BryanKadzban]

OK, let's see if that compiles for the xtensa chips. Hopefully github still runs the workflows (I don't have the xtensa rust fork easily available locally ... I'll have to get it if the workflows don't trigger).

[BryanKadzban]

OK, no async support in this rev, but everything else is there. Mind looking once more? Thanks!

[BryanKadzban]

And oh, right, I forgot to mention this -- I don't have access to any of the ESP chips anymore, for the next couple of months. I think the interrupt flags work the way I'm expecting (they get set regardless of whether the interrupt is mapped through the interrupt matrix to the CPU and handled), but I could be wrong on that. If running this on an actual device that has GPIO 4 connected to an LED, flips back and forth between 0 and 100 but doesn't actually fade, then this might be the reason, and is_duty_fade_running() will need some changes.

[bjoernQ]

Maybe it's just me but I tried the ESP32 and ESP32-C3 example without any success. (The LED doesn't light up at all

Also, in development mode I get this (on ESP32-C3)

!! A panic occured in 'C:\projects\review\kadzban\esp-hal\esp-hal-common\src\ledc\channel.rs', at line 770, column 33

PanicInfo {
    payload: Any { .. },
    message: Some(
        attempt to subtract with overflow,
    ),
    location: Location {
        file: "C:\\projects\\review\\kadzban\\esp-hal\\esp-hal-common\\src\\ledc\\channel.rs",
        line: 770,
        col: 33,
    },
    can_unwind: true,
}

[BryanKadzban]

Well, I don't even have to read the channel.rs:778 code to know what that panic is from: it's the end-start subtraction, when the fade is going in one of the two possible directions. It needs to figure out an absolute value.

And after looking at the code, I see another bug in that area too, in the while loop's condition. That's probably what causes the LED to not light up in release mode. (Certainly the u32 overflow isn't working right in release mode ... I haven't thought all the way through what the hardware would do, but I bet it's the reason.)

Fixed both of those.

[bjoernQ]

Thanks for the fix

A quick test on ESP32-C3:
The LED lights up at 100% and stays there. I see the while loops end really, really quick

I understand this might be hard to debug without access to hardware

[BryanKadzban]

I assume you mean that the is_duty_fade_running while loop is exiting quickly, right? There is one more inside start_duty_fade! ... but it's intended to exit after only a few iterations. So I suspect you meant the one calling is_duty_fade_running.

Do you happen to have an oscilloscope that you could hook up to the LED output? Is it flickering off for a short time, by any chance, or is it on 100%?

I guess the other question is, is the code programming some wrong values into the duty-cycle-fading registers that makes the fade-complete interrupt trigger immediately, or is the interrupt status bit working in some way I don't expect. Can you tell if the is_duty_fade while loop is running even twice?

So with a 5-bit duty-cycle resolution (32 values), a fade from 0 to 31 over two seconds would wait around 62-63 milliseconds per value (1/16th of a second). The code calculates a value of 2048 of the 24kHz overflow rate, which comes out to about 85ms. That's not that far off, especially considering it then only counts up 23 of them instead of 32.

(...That actually points out a shortcoming of that while loop in the start_duty_fade! macros. It only picks powers of 2 ... doing some integer division should be both constant-time, and more accurate. I'll have to work through what to divide out to get the right values out, but the target here is somewhere around 1550 instead of 2048, and 31 instead of 23.)

But that indicates the register values might be fine, and maybe it's the interrupt bit that doesn't work right. :-/ I might have to register an interrupt handler even without the async support, just to know when the "sync" version is finished. (It's async in the hardware either way...)

[bjoernQ]

Ok I took some time to experiment with it a bit.

Looking at the signal with a logic-analyzer revealed the duty cycle is indeed changing but too fast for the human eye to notice.

I changed the frequency to 100u32.Hz() and can see some "pumping" when changing the 2000 to 20000
Seems the lowest it goes to is a duty-cycle of 3% (from 100 to 0)

So, the duration_ms apparently isn't ms

[BryanKadzban]

That 3%-instead-of-0% is probably a result of the while loop going a full power of two each time; it doesn't end up at quite the right final count to get all the way to the zero that the code asked for. Fixing it to be a plain division ought to sort that out too.

The speed of duty-cycle fading should be independent of the frequency, or at least that was my intention. I wonder whether changing the duration without changing the frequency would have had the same effect ... but don't bother testing it; I think either changing the loop to an explicit division is likely to improve things, or the bug is a missing factor somewhere in the whole calculation (i.e. I misunderstand something in the reference manual).

I wonder if setting a duration of 32000 ms would result in a one-second fade? (I wonder if I'm just off by a factor of the 32 steps of PWM resolution, somewhere?)

Sadly I've also lost access to the local git repo where I was working on further changes for a while (hopefully only a couple days, we'll see), so I can't easily push another commit to my fork. I'll get to it, it just might be a bit.

[BryanKadzban]

OK, I hope this changes things. It should certainly fix the 3% instead of 0% issue, but I'm not sure if it will sort out the duration correctly.

Assuming it passes the workflows, give it a try. I've renamed a bunch of the variables so I could keep it more clear what they were. If it's still a too-short fade duration, I'd be interested to find out whether it's off by a factor of exactly 32, or 10, or if you can even tell for sure. (Maybe the logic analyzer could help?) Also, I'd be interested if it's possible to tell whether it's running too short of a time between steps (and so the number-of-overflows-between-duty-cycle-increments is too small), or if it's running too few steps overall. Or, actually, if it's incrementing the duty value by too much each step; that's possible too.

Thank you again for doing the remote debugging! :)

[bjoernQ]

Already looks better - at least on ESP32-C3 there is now a visible fade effect.

Looking with the LA the full fade (fade-in and fade-out) is approx. 725556 ns when passing the 2000ms in.
With 1000ms it is 600629 ns
With 3000ms it is 1027 µs

[BryanKadzban]

That's odd - I would expect the result to be linear with the passed-in duration, and I'd expect the line to go through the origin (that is, a 0 duration_ms seems like it should result in a zero length fade, even if the factors are all wrong). The 2000 and 3000 cases line up with that fairly well (725 * 1.5 is 1088), but the 1000 case definitely does not.

Well, maybe more info would help. Can you post an image of the logic analyzer's capture of the 2000 or the 3000 run, or both? From the calculations I'm doing, at 2000, the LEDC is supposed to run 1548 on/off cycles (at 41.6667 microseconds each, i.e. 1/24000Hz) at each of the 32 possible duty cycle values that the 5-bit resolution provides. Clearly it's not doing that if the run finishes in 360-whatever milliseconds instead of 2 seconds, but I wonder how many cycles it does run at each of the 32 values. Or if somehow it's doing an increment of 1548 and only doing two on/off cycles, or something weird like that.

I do see something odd though: when setting the duty cycle field in the non-fade case, I've just realized that the code is shifting the calculated lpoint value left by 4 bits for some reason. I don't see why lpoint should be multiplied by 16 though ... but that might be at least part of the issue. Do you know where those 4 bits are coming from? Is the clock somehow 16x faster than it looks from the timer.rs code?

Thanks again!

[bjoernQ]

captures.zip

Here are three captures in Saleae 2.4.7 format. It's only one full cycle (fade-in / fade-out) for each 1000ms, 2000ms and 3000ms

[BryanKadzban]

So I've pulled down the demo version of Saleae's UI. The 2000.sal file is showing nothing happening except one dip from 1 to 0, then back to 1 at 250ns later, starting at about 337ms. It stays at 1 until a 121us dip to 0 at ~519.5ms. After that it starts to do some PWM; the first cycle starts at 519.65275ms. It almost looks like it's fading from 100% to 0% first, and the 121us dip is when the code first sets the duty value to zero when the channel is initialized? Maybe that doesn't matter much though.

The overall oscillation is definitely switching between a 41.5us and 41.75us period, which lines up pretty well with the expected 41.66667us period from 24kHz, and the reference manual's description of how the fractional divisor works. So at least that's set up right. 1/32 of that (the expected PWM resolution) is 1.29 or 1.3 microseconds, and the logic analyzer is showing a dip-to-zero that's 1.25 microseconds wide for the first duty step, so that lines up relatively well too (I will naively assume the difference is the logic analyzer's sample rate; everything appears to happen at a quarter-microsecond boundary).

The issue appears to be that the high time changes from 40.25/40.5 microseconds, to 39/39.25 microseconds, starting at 541.4885ms into the capture. That means the first duty-cycle is effective for 21835.75us, or ... 524 iterations (if I assume the averaging works out to 41.66667us). I expect 1548 iterations, so that's around 1/3 what it should be. And the last one-to-zero drop happens at 1196.5215ms, for a total fade-out time of 677us, which is 31 steps, so that's pretty close. (If we add an all-ones step or all-zeros step at one end or the other, which the LA won't show, we'll get the expected 32 steps.)

But 1/3 is a weird factor to be off by.

I'll have to see what else I can find. Just updating here to explain what I've found so far.

[BryanKadzban]

Well there's the problem.

Looking at the difference between 524 and 1548, instead of the ratio, comes out to 1024 - and that does look like a suspicious number.

And, aha, the duty_cycle, duty_num, and duty_scale fields in the LEDC's conf1 register are only 10 bits wide, not 16, so the value the code is writing is getting truncated at ten bits - losing the 1024 from the eleventh bit. (And possibly messing with the bottom bit of the duty_num field, depending on the code generated by svd2rust.)

So there's a max of 1024 repetitions of any particular duty cycle - and the scale field has a min of 1, so with a 5-bit resolution timer, the max overall duration is 32767 cycles, or about 1.3 seconds.

That doesn't explain the 1000ms duration run, but I'll have to look closer at its logic analyzer trace to see if I can find anything wrong.

I also figured out the 4 bit shift - re reading the reference manual showed that those bottom 4 bits are used for a fractional duty cycle (some cycles use duty n, others use duty n+1). There's nothing indicating the same behavior for the conf1 fields used to do the fading.

It'd be nice if the type system could prevent the problems with too large durations (alternately, too small resolution, or too high frequency). Or if a couple of the bits in the scale register could be applied to the bottom 4 bits of the duty register to set a fractional lpoint; having a scale of 1/4th would mean changing the duty cycle 4x as frequently to keep the same overall duration, which works fine for 2s.

But I'm thinking I'll have to change those 65535 checks (and try_from / unwrap_or) to check against 1023 instead. At that point it might be best to add another couple of variants in the error enum, as well. It may take a bit to get those pushed but we'll see.

[BryanKadzban]

Yeah, just got a chance to look at the 1000.sal file, and the "down" fade runs from 461.431ms to 1461.251ms, or just shy of one second. So it looks to me like that duration is pretty close to the intent - am I missing anything?

[BryanKadzban]

I think that should fix it. The examples now run a 1-second fade down, then 1-second fade up; they also assert that a 2-second fade would return an error.

[BryanKadzban]

Would it be better to rebase this onto current head before committing?

Also, would it be better to rebase this set of 12 commits into just one or two, skipping all the broken stages in between?

[bjoernQ]

Great! This now works as intended on all chips except ESP32-C6 - I attached a LA capture
ESP32-C6.zip

I usually squash all commits and rebase. I think squashing this to one or two commits would be definitely a good thing

[BryanKadzban]

Reading through the -c6 reference manual again, I see an extra step that its gamma capability requires, that the code probably isn't doing (I think it could be done at channel setup time, but I'm not sure ... it's definitely not happening at duty cycle fade time).

I'll try to add that tomorrow, if I get a chance; hopefully that's all it will need, so you can run one last test, I can squash and rebase, and we can get this support added. :)

[BryanKadzban]

Got a chance to add it tonight. :) Not sure if you'll look at this on the weekend, but let me know if it still doesn't work right on the -c6.

[bjoernQ]

Thanks for looking into this.

Seems like is_duty_fade_running now always returns true on C6 so it never gets past while channel0.is_duty_fade_running() {} - I also don't see much happening on the pin (33 pulses and then it stays low)

[BryanKadzban]

Ok, I'm not sure if that's the only issue, but it's possible. The old code was doing a read-modify-write sequence on the new register to set the count of gamma sequences, but the register contains a bunch of reserved bits, a pair of bits to pause and resume fading (writing a 1 to one or the other of them does the pause or resume), and the bits for the count.

It's not documented what value gets read from the pause and resume bits, but if for some reason the pause bit reads as a 1, then the modify won't change it, and the code will trigger a pause.

So I changed it to call write() instead, and always set the other bits to their SVD default value. Hopefully that was the problem - let me know if it works any differently now, and if it's still broken I'll keep looking at the reference manual.

I am curious if those 33 pulses were all the same duty cycle - did it change at all in that short time?

[bjoernQ]

Unfortunately, this doesn't change anything (besides I only see 28 pulses) - I added the LA capture of running the example without running the fading in a loop

esp32-c6.zip

[BryanKadzban]

OK, one more step I missed in the reference manual. Let's see if this last change helps.

Looks like the workflows aren't happy because I didn't add an entry to the change log - looks like it's time for a rebase, since there is no change log file in this branch. :) While I'm working on that, let me know if this makes any difference.

[BryanKadzban]

There we go. (I didn't expect to get time to finish the rebase this soon, but here we are.) Let me know if this works.

[bjoernQ]

🚀 this looks good now! Thanks for the effort
I will test on all targets during the day and then it should be fine to merge

[bjoernQ]

I guess you can remove this limitation about the ESP32-C3

[bjoernQ]

Works fine on all tested targets 👍Just one outdated comment - otherwise this should be fine

[BryanKadzban]

Sorted out the comment, as well as the conflict in the change log file.

If the workflows pass, I think this looks good too.

Thank you for all the remote hardware testing!

[MabezDev]

Thanks, @BryanKadzban, really nice work. Looks good from my perspective :)

[bjoernQ]

LGTM! Thanks for your contribution