Board config: timer config simplifications

[bkueng]

Having done another PX4 board port, the most tedious, error-prone and redundant part I find is the timer configuration. This PR aims at improving that, with generality in mind.

Summary

A current timer + channel config looks like this:

	{
		.base = STM32_TIM1_BASE,
		.clock_register = STM32_RCC_APB2ENR,
		.clock_bit = RCC_APB2ENR_TIM1EN,
		.clock_freq = STM32_APB2_TIM1_CLKIN,
		.first_channel_index = 0,
		.last_channel_index = 3,
		.handler = io_timer_handler0,
		.vectorno =  STM32_IRQ_TIM1CC,
		.dshot = {
			.dma_base = STM32_DMA2_BASE,
			.dmamap = DMAMAP_TIM1_UP,
			.start_ccr_register = TIM_DMABASE_CCR1,
			.channels_number = 4u /* CCR1, CCR2, CCR3 and CCR4 */
		}
	},

	{
		.gpio_out = GPIO_TIM1_CH4OUT,
		.gpio_in  = GPIO_TIM1_CH4IN,
		.timer_index = 0,
		.timer_channel = 4,
		.ccr_offset = STM32_GTIM_CCR4_OFFSET,
		.masks  = GTIM_SR_CC4IF | GTIM_SR_CC4OF
	},

Accompanied with several defines in the header in the form #define GPIO_TIM12_CH2OUT /* PH9 T12C2 FMU8 */ GPIO_TIM12_CH2OUT_2.

The PR reduces all of it to 2 lines, one for the timer definition:

initIOTimer(Timer::Timer1, DMA{DMA::Index2, DMA::Stream5, DMA::Channel6}),

and one for the channel:

initIOTimerChannel(io_timers, {Timer::Timer1, Timer::Channel4}, {GPIO::PortE, GPIO::Pin14}),

Details

• the 1. commit breaks the cyclic dependency between the io_timers_t and timer_io_channels_t data structures, and removes some fields that are not required or we can easily get at runtime.
• general removal of the use of GPIO_GPIOx_OUTPUT, and usage of timer_io_channels instead. The data structure stays otherwise the same, but instead of explicitly setting each field, constexpr methods are used for initialization during build time. This allows to specify only the information that is really required (i.e. timer, channel and pin), while ending up with the same binary.
• some architecture-specific mapping code is required to map from the specified configuration (i.e. Timer::Timer1 to the NuttX defines (the ones that are currently used).
• requires C++14 for the constexpr code ( update C++ STD from c++11 to c++14 #13811)

Benefits

• specify only the details that are required
• while still allowing for full flexibility. The existing method of explicitly initializing everything could still be used. But customizations can also be done much nicer, and more specific, see v4 (pulldown, da38cd0) or crazyflie (different clock frequency, 4d609bd) for example.
• complex compile-time checks are possible, for example ensuring that all channels of a certain timer are grouped together.

Drawbacks

• even though the method is generic, and could be used for SPI for example, it cannot be used for the NuttX board.h config, as it requires C++. Overall I still think it's worth exploring a script-based generator (including for the NuttX defconfig).

Bugs

While working through this, I found 2 configuration bugs:

• nxp/fmuk66-v3: the 2. timer has 3 channels associated not 2
• crazyflie: TIM2 channels are not all grouped together. The current data structure does not allow for this. This issue is still unresolved and I see several options:
  • use a bitfield instead of first_channel_index and last_channel_index, or remove the fields altogether. The io_timer_handler IRQ handler would become slightly slower.
  • use a custom mixer to account for this (assuming we won't have more boards with that issue)
  • add a board-specific channel reodering

@davids5 @dagar

[davids5]

@bkueng - this looks interesting. I do not understand how the association of the IP blocks that are need to support a timer are going to be done.

Like where KINETIS_SIM_SCGC6 is the place that FTM0 on a K66 is enabled by what bit is not the same for an FTM3 etc.

Where are cascaded clocks handled? (see the IMX rt branch)

ISRs should be short as possible. So not calculating (or shifting) at run time is preferred. How will that be done?

[dagar]

• crazyflie: TIM2 channels are not all grouped together. The current data structure does not allow for this. This issue is still unresolved and I see several options:

  • use a bitfield instead of first_channel_index and last_channel_index, or remove the fields altogether. The io_timer_handler IRQ handler would become slightly slower.
  • use a custom mixer to account for this (assuming we won't have more boards with that issue)
  • add a board-specific channel reodering

A custom mixer for the crazyflie sounds fine, it's already a weird target. Last I heard it's not even working in master (I don't have one to test).

[davids5]

@bkueng - please have a look at the IMXRT and suggest how this can be applied to it.

[bkueng]

A custom mixer for the crazyflie sounds fine, it's already a weird target.

I've done that.

@bkueng - please have a look at the IMXRT and suggest how this can be applied to it.

@davids5 I have merged the RT branch and done the changes (not quite finished yet) in bkueng@d363393 and bkueng@b5b6826. The main difference is that the IOMUX PAD and GPIO port + pin are independent. The timer setup and config could also be more generalized (also with cascaded clocks if needed), but I did not look into that.
Let me know what you think.

ISRs should be short as possible. So not calculating (or shifting) at run time is preferred. How will that be done?

All ISR routines are unchanged in terms of runtime.

If everything looks good I'll continue and update all boards.

[davids5]

A custom mixer for the crazyflie sounds fine, it's already a weird target.

I've done that.

@bkueng - please have a look at the IMXRT and suggest how this can be applied to it.

@davids5 I have merged the RT branch and done the changes (not quite finished yet) in bkueng@d363393 and bkueng@b5b6826. The main difference is that the IOMUX PAD and GPIO port + pin are independent. The timer setup and config could also be more generalized (also with cascaded clocks if needed), but I did not look into that.
Let me know what you think.

ISRs should be short as possible. So not calculating (or shifting) at run time is preferred. How will that be done?

All ISR routines are unchanged in terms of runtime.

If everything looks good I'll continue and update all boards.

@bkueng - can we have a review of this on the dev call or some other time this week?

[bkueng]

Sure

[davids5]

@bkueng - Now that I understand the genius in this I have a great appreciation for the work you have done herein.

As discussed I will rebase and bring the RT in this week, then you can bring this in on top of it.

Thank you!

[codecov]

Codecov Report

Merging #13871 into master will decrease coverage by 0.29%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master   #13871     +/-   ##
=========================================
- Coverage   52.98%   52.69%   -0.3%     
=========================================
  Files         625      605     -20     
  Lines       52991    51256   -1735     
=========================================
- Hits        28079    27008   -1071     
+ Misses      24912    24248    -664

Flag	Coverage Δ
#mavsdk	?
#python	100% <ø> (+96.93%)	⬆️
#sitl_mission_FW	100% <ø> (?)
#sitl_mission_MC_box	29.83% <100%> (?)
#sitl_mission_MC_offboard_att	100% <ø> (?)
#sitl_mission_MC_offboard_pos	28.56% <100%> (?)
#sitl_mission_Rover	26.3% <0%> (+0.94%)	⬆️
#sitl_mission_VTOL_standard	33.79% <100%> (?)
#sitl_mission_VTOL_tailsitter	33.82% <100%> (+0.99%)	⬆️
#sitl_python_FW	29.32% <0%> (+1.03%)	⬆️
#sitl_python_MC_box	100% <ø> (+70.93%)	⬆️
#sitl_python_MC_offboard_att	100% <ø> (+72.35%)	⬆️
#sitl_python_MC_offboard_pos	28.64% <100%> (+0.8%)	⬆️
#sitl_python_Rover	100% <ø> (?)
#sitl_python_VTOL_standard	33.86% <100%> (?)
#sitl_python_VTOL_tailsitter	100% <ø> (+67.1%)	⬆️
#unittest	36.98% <100%> (+2.97%)	⬆️

Impacted Files	Coverage Δ
src/lib/mixer/MultirotorMixer/MultirotorMixer.cpp	84.06% <100%> (ø)	⬆️
src/modules/navigator/takeoff.cpp	3.92% <0%> (-76.48%)	⬇️
...flight_tasks/tasks/Failsafe/FlightTaskFailsafe.cpp	0% <0%> (-73.92%)	⬇️
src/modules/navigator/rtl.cpp	14.46% <0%> (-44.22%)	⬇️
...lib/flight_tasks/tasks/Manual/FlightTaskManual.cpp	0% <0%> (-38.89%)	⬇️
src/modules/navigator/loiter.cpp	8.16% <0%> (-38.78%)	⬇️
Tools/px4airframes/xmlout.py	98.18% <0%> (ø)	⬆️
src/lib/parameters/px4params/srcparser.py	86.87% <0%> (ø)	⬆️
src/modules/sensors/vehicle_imu/VehicleIMU.cpp	43.87% <0%> (-33.4%)	⬇️
src/modules/simulator/simulator_mavlink.cpp	59.31% <0%> (-19.06%)	⬇️
... and 97 more

[bkueng]

This is now getting ready, I updated all boards.
Tested so far on Pixracer, both PWM + DShot.

@davids5 once the imxrt is in, I'll rebase once again and update that as well.

@PX4/testflights can you test this on all boards that you have?

[davids5]

@bkueng - So that I am clear, moving forward the defconfig will not state what HW Timers are enabled if PX4 is driving them as PWM ?

I am guessing it was inconsistent?

What about UAVCAN, HRT etc?

The thing we will need to look out for is a HW timer that has shared usage. Assume A and B a drivers use TIM1.
A & B had the timers enabled in the RCC before either driver runs and the start up ordered or A and B did not matter. Now all drivers need to do the ENR enable AND not use the RST in the rcc on a shared timer.

The former MAY manifest it's self as a hardfault (buss fault on not enabled IP block), the more subtle failure will be Wrong (or reset) values in registers.

We should leverage all the goodness you have added to get a compilation time HW resource usage/contention map.

[bkueng]

Yes it was inconsistent, and it was also never clear to me when I ported a board if it was needed/harmful/does not matter.

HRT also has a check (# error must not set CONFIG_STM32_TIM1=y and HRT_TIMER=1), for UAVCAN I don't know.
I agree we should consolidate that further, especially where overlapping use is possible.
From what I saw, we or NuttX does not init any timers by just enabling the NuttX timer config, but I may have overlooked something.

[davids5]

@bkueng

not init any timers

That is true if the driver are not included via defconfig.

RCC is it. An example of the RCC stuff is here. is here. https://github.com/PX4/NuttX/blob/px4_firmware_nuttx-8.2/arch/arm/src/stm32/stm32f40xxx_rcc.c#L332-L386

[Junkim3DR]

Tested on NXP FMUK66 v3

Modes Tested

• Position Mode: Good.
• Altitude Mode: Good.
• Stabilized Mode: Good.
• Mission Plan Mode (Automated): Good.
• RTL (Return To Land): Good.

Procedure
Arm and Take off in position mode, after flying for approximately one minute, switched to altitude then stabilized mode proceed to switch to mission plan mode then make sure that vehicle follows all waypoints as shown in QGC, once completed all waypoint then trigger RTL.

Notes
No issues noted, good flight in general.

Log

• https://review.px4.io/plot_app?log=24dab5c2-ee6f-4e5a-aee9-a1436b7deb10

[dagar]

The NXP i.MXRT changes (#13678) have merged to master.

[bkueng]

Updated for imxrt - it is really not SW-developer friendly.
I validated that the generated timer/channel data structure is binary-equivalent to before, so I expect it to work.

[Tony3dr]

We are encountering issues with CUAV Nano V5 and Pixhawk 4 V5. The firmware is broken for those vehicles, no connection on the vehicles doesn't detect the ESC's, see image below:

@bkueng

[jorge789]

Tested on PixRacer V4

Indoor test flight.
Mode tested on Stabilized

Takeoff and landing on Stabilized

Log: https://review.px4.io/plot_app?log=b797910e-f15b-433a-9b52-1167a364d0de

[davids5]

@bkueng - I have not tested on RT, but that should not stop this from coming in,