[Soft PWM] Handle the PWM duty change events inline.

[viesturz]

PWM change being handled in a separate timer results in a ripple in the signal because the timings of the current cycle are forgotten. Especially visible as servo jitter.

This adds an inline update when only PWM cycle is changed, ensuring consistent cycle length and on/off timings.

[KevinOConnor]

Thanks. I'm not sure what the intent of this change is, but at first glance I don't think it should be implemented this way. At a high-level, it is the role of the host to schedule actions, and not the role of the mcu to judge that schedule.

If there is a concern with scheduling of servo updates, then the host can track when it has last issued an update, and arrange for the next update to be at an upcoming pin disable time. Take a look at klippy/extras/bltouch.py for an example of this.

Cheers, -Kevin

[viesturz]

Thanks. That should work as well.

To reiterate my problem is that whenever I set a new value to a servo.py, it occasionally jumps forward before moving to the set angle. Especially visible when repeatedly setting the angle to perform a speed controlled sweep. I traced this down to the change being scheduled over a pin already on state, resulting pin on time up to doubling for the cycle.

From what I can tell this affects all pwm pin users and has more impact on the slower switchers like bed heaters. So looking for a fix that would address all uses. Will see if I can tweak MCU_pwm to the same effect.

Best,

Viesturs

On Mon, Sep 30, 2024, 18:52 KevinOConnor @.***> wrote:

Thanks. I'm not sure what the intent of this change is, but at first glance I don't think it should be implemented this way. At a high-level, it is the role of the host to schedule actions, and not the role of the mcu to judge that schedule.

If there is a concern with scheduling of servo updates, then the host can track when it has last issued an update, and arrange for the next update to be at an upcoming pin disable time. Take a look at klippy/extras/bltouch.py for an example of this.

Cheers, -Kevin

— Reply to this email directly, view it on GitHub https://github.com/Klipper3d/klipper/pull/6698#issuecomment-2383704534, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAMQT47IS4PWNKMK7PS6HRTZZF6TLAVCNFSM6AAAAABPASC3ACVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGOBTG4YDINJTGQ . You are receiving this because you authored the thread.Message ID: @.***>

[KevinOConnor]

FYI, heater updates are already tightly synchronized. I don't think generic output_pin (nor similar) should introduce synchronization as it's not clear all users would want that. I can see where it would be useful for servo.py though.

Cheers, -Kevin

[github-actions[bot]]

Thank you for your contribution to Klipper. Unfortunately, a reviewer has not assigned themselves to this GitHub Pull Request. All Pull Requests are reviewed before merging, and a reviewer will need to volunteer. Further information is available at: https://www.klipper3d.org/CONTRIBUTING.html

There are some steps that you can take now:

1. Perform a self-review of your Pull Request by following the steps at: https://www.klipper3d.org/CONTRIBUTING.html#what-to-expect-in-a-review If you have completed a self-review, be sure to state the results of that self-review explicitly in the Pull Request comments. A reviewer is more likely to participate if the bulk of a review has already been completed.
2. Consider opening a topic on the Klipper Discourse server to discuss this work. The Discourse server is a good place to discuss development ideas and to engage users interested in testing. Reviewers are more likely to prioritize Pull Requests with an active community of users.
3. Consider helping out reviewers by reviewing other Klipper Pull Requests. Taking the time to perform a careful and detailed review of others work is appreciated. Regular contributors are more likely to prioritize the contributions of other regular contributors.

Unfortunately, if a reviewer does not assign themselves to this GitHub Pull Request then it will be automatically closed. If this happens, then it is a good idea to move further discussion to the Klipper Discourse server. Reviewers can reach out on that forum to let you know if they are interested and when they are available.

Best regards, ~ Your friendly GitIssueBot

PS: I'm just an automated script, not a human being.

[nefelim4ag]

There are more elegant ways to fix this for servos, if there is a will to change MCU, just ensure the maximum on_duration will be enough, like this:

diff --git a/src/gpiocmds.c b/src/gpiocmds.c
index dac2f008..38cd8344 100644
--- a/src/gpiocmds.c
+++ b/src/gpiocmds.c
@@ -14,6 +14,7 @@
 struct digital_out_s {
     struct timer timer;
     uint32_t on_duration, off_duration, end_time;
+    uint32_t on_rem;
     struct gpio_out pin;
     uint32_t max_duration, cycle_time;
     struct move_queue_head mq;It does not provide a significant difference at least for my LEDs, when new PWM arrives out of sync, it still flickers, maybe slightly less.

@@ -46,6 +47,9 @@ digital_toggle_event(struct timer *timer)
     if (d->flags & DF_CHECK_END && !timer_is_before(waketime, d->end_time)) {
         // End of normal pulsing - next event loads new pwm settings
         d->timer.func = digital_load_event;
+        d->on_rem = d->end_time;
+        if (d->flags & DF_ON && timer_is_before(d->end_time, waketime))
+            d->on_rem = waketime;
         waketime = d->end_time;
     }
     d->timer.waketime = waketime;
@@ -102,6 +106,10 @@ digital_load_event(struct timer *timer)
         return SF_RESCHEDULE;
     }
     uint32_t waketime = d->timer.waketime + on_duration;
+    if (timer_is_before(d->timer.waketime, d->on_rem)
+        && timer_is_before(d->on_rem, waketime))
+        waketime = d->on_rem;
+
     if (flags & DF_CHECK_END && !timer_is_before(waketime, end_time)) {
         d->timer.waketime = end_time;
         return SF_RESCHEDULE;

There is still a possibility of short off_duration between on events, but I'm not sure how severe it is.

---

It does not provide a significant difference at least for my LEDs, when new PWM arrives out of sync, it still flickers, maybe slightly less.