MINIMUM_STEPPER_PULSE to MINIMUM_STEPPER_PULSE_NS

[mh-dm]

Description

Switch from MINIMUM_STEPPER_PULSE (in µs) to MINIMUM_STEPPER_PULSE_NS (in nanoseconds). This PR is basically just code refactoring. Note that MINIMUM_STEPPER_PULSE is in Configuration_adv.h and if left undefined it is automatically configured by Conditionals_adv.h based on the used stepper drivers.

Benefits

Cleaner code for handling stepper motor drivers that can do sub microsecond step pulse widths (like LV8729 and TMC2xxx). Allows easier user experimentation with sub microsecond step pulse widths.

[thisiskeithb]

Since a config option has changed / been renamed, you’ll need to add it to Changes.h.

[mh-dm]

Thanks, done.

[thinkyhead]

Some of these things defined as hal_timer_t should probably be changed to uint32_t. Both AVR and HC32 use 16-bit timer width, so it's very likely to overflow. If we're curious, we could add tests first for to see if any hal_timer_t value is currently overflowing HAL_TIMER_TYPE_MAX and add a build test with plausible high step time values to see if that leads to overflow.

[thinkyhead]

Note that this is only 1/4 of the full F_CPU speed, but it's unclear whether we can run interrupt timers at such a high resolution as 1/1, or that we need to. All counts would require two extra bits.

Since F_BUS_ACTUAL is volatile, perhaps the system clock speed is able to change dynamically, or maybe this is only done under program control. If Marlin is able to keep the clock at a constant rate then we should be ok just using F_BUS, which is provided by the Teensy platform. I'm not sure yet how F_CPU and F_BUS are defined, since you can change the CPU speed with PlatformIO option board_build.f_cpu. If Teensy isn't picking up board_build.f_cpu then we may add our own -DF_CPU or some other define that Teensy platform is expecting.

[mh-dm]

Some of these things defined as hal_timer_t should probably be changed to uint32_t. Both AVR and HC32 use 16-bit timer width, so it's very likely to overflow. If we're curious, we could add tests first for to see if any hal_timer_t value is currently overflowing HAL_TIMER_TYPE_MAX and add a build test with plausible high step time values to see if that leads to overflow.

hal_timer_t is for timer ticks which is usually cycles/4. And given that most things changed look like cycles they should indeed be uint32_t instead.

If there is indeed some math / comparison being done between timer ticks/hal_timer_t and cycles/uint32_t without a clear conversion being done then that's revealing of a bug. If my PR didn't introduce it I recommend it be addressed in a separate PR since this one is already getting too big for my comfort :).

[mh-dm]

I did a pass over everything at 92fe629 commit and it looks good.

[thinkyhead]

I did a pass over everything…

Thanks for the review. If more variables, methods, and parameters should be working with uint32_t (especially under known circumstances) we can certainly apply that. For AVR we generally want to use the smallest types required, so that is specifically where hal_timer_t comes in. It generally makes sense to convert #define values to 32-bit and let the compiler decide how to deal with type conversion. Any of the constexpr uint32_t defined with a value under 65536 could be changed to uint16_t and that may be better for some code on AVR, or it may make no difference. As we are further fixing up and optimizing the motion code with these PRs I won't sweat that small detail until some profiling shows that it matters.