Cartographer (STM32F042 HW PWM as CLK OUT)

[nefelim4ag]

The goal: be able to output a timer/high-frequency clock on the pin.
TLDR, Cartographer uses the PB4 pin as CLK IN on the LDC1612.

I'm not sure if it is possible or if it makes sense to do the same for other boards, or, though, to implement a different (from PWM) command.
For now, the current code for STM32F042 would allow us to set the frequency as high as 48_000_000 / 256 = ~187500.
Or (half of that, I'm still a little bit puzzled with PWM registers).
Any smaller value would be technically NoOp.

Technically, it is possible to adjust the MAX_PWM if the cycle time is too low, which I do in the PR.
That can create confusion if we try to set the PWM value larger than the new MAX_PWM.
As there is no way to actually feed back the value.
(Well, I can add feedback for that, probably: sendf("config_pwm_out oid=%c max_pwm=%u",...);)
But I hope that, for now wrong configuration should be handled by shutting down on a value larger than the actual MAX_PWM.

I tested it with an oscilloscope and the following snippet:

[output_pin carto_clk_in]
pin: PB4
pwm: True
hardware_pwm: True
value: 1
scale: 256
# 0.000000168 ~ 6Mhz
# 0.000000042..84 ~ 12Mhz
# 0.000000021 ~ 24Mhz
cycle_time: 0.000000021

If I try to set a larger value, it will fail as expected, with:

    if (val > g.max_pwm)
        shutdown("PWM invalid value");

STM32F1/F401 seems to have a lower FREQ_PERIPH value, so I guess they are slightly more susceptible to hitting this code path.

Hope that sounds sane.

[KevinOConnor]

Interesting. Unless I'm missing something though, this change may break other users of hard_pwm. The current hard_pwm interface assumes that "cycle_time" is approximate while pwm duty times are accurate. This seems to change that (cycle_time may be more accurate, but some pwm duty cycles can raise a shutdown).

FWIW, have you considered adding a new DECL_COMMAND("stm32_timer_output cycle_ticks=%u on_ticks=%u") type command to src/stm32/hard_pwm.c as an alternative? It may be easier to add a new [gpio_clock_output] type config section than to make hard_pwm due what you are looking for.

Cheers,
-Kevin

[nefelim4ag]

Interesting, I never thought about it in an accurate/approximate way.
It is possible, though. If, for some reason, user do ask for high frequency, which is not actually provided, eg +100kHz.
It would downscale the PWM accuracy.

Hmm, I somewhat assumed that it is better to avoid adding entities because this would multiply the amount of work required (no entity - magic behavior, feedback 1 entity, general timer interface - many entities).
I did not think that I could actually do the hw (like stm32 only) specific command here.

So, I'm cool with that, I will update.

Thanks.

[nefelim4ag]

Now it should look better.

[gpio_clock_output carto_clk_in]
pin: PB4
pulse_width: 0.5
frequency: 9600000

Also works as expected. (I realized that max_pwm == cycle_time).

Thanks.

[KevinOConnor]

Thanks. It seems fine to me. The only thing I'm not sure on is the name "gpio_clock_output" - I know I suggested that name, but I'm unsure of my suggestion. Lets give a few days and see if there are other thoughts.

-Kevin

[nefelim4ag]

In my head, there are only 2 possible users for now: Carto and IDM (old rp2040 carto).
I guess if we add it, it could be used somewhere where I would not expect.

So, maybe it could be renamed as: static_clock_output, similar to static_digital_output?

Thanks.

[KevinOConnor]

Another possibility might be "pwm_clock" (thus being similar to "pwm_tool" and "pwm_cycle_time").

Cheers,
-Kevin

[nefelim4ag]

I guess the current pwm_tool/pwm_cycle_time implies that it is configurable from runtime with SET_PIN PIN=.. VALUE=...
Not like it is technically not possible to do so.

-Timofey

[github-actions]

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]

Renamed to static_pwm_clock to the benefit of neither party :D.

Some additional logging and removal of redundant value inspired by @garethky.

Small host side bug fixes, because I somehow manage to diverge and push broken one commits in the first place. Shame on me.

-Timofey

[KevinOConnor]

Thanks. In general it seems fine.

Why does PrinterClockOutputPin() create a ppins.setup_pin('pwm', config.get('pin')) ? Wouldn't that cause the pin to be initialized twice - once as a pwm pin and then again as a clock output? At first glance, I wonder if it would be simpler if the constructor called ppins.lookup_pin() and stored the relevant parameters (pin_params['mcu'], pin_params['invert'], and pin_params['pin']) directly.

-Kevin

[nefelim4ag]

Thanks,
I just didn't think about it.

Fixed. Not sure about practical usage of inversion, but let it be.

-Timofey

[KevinOConnor]

Ah, yeah, I guess 'invert' doesn't make much sense if it's 50/50 duty cycle. Not a big deal either way.

Probably best to remove the now unused mcu_pwm.get_pin() method.

Otherwise, let me know when you are ready and I will commit.

Thanks,
-Kevin

[nefelim4ag]

Oops, Fixed.

Tested one last time, it is ready on my side.

Thanks,
-Timofey

[KevinOConnor]

Thanks. I committed this along with a minor change in 7377da6 on top.

-Kevin