[Bug] Modules do not react on parameter updates anymore

[MaEtUgR]

Describe the bug

After 4a55393 from #22881 the part that's called when the parameter_update topic gets updated does not get called anymore. This can be worked around by stopping a module and rerunning it, after doing so it works as expected again.

I tested that with two instances:

1. px4io module on fmu-v5x hardware. I needed to change PWM_MAIN_DIS1 and it did not apply anymore until I restarted the driver.
2. mc_pos_control module in SITL SIH (make px4_sitl sihsim_quadx) and just added a printf here:

   PX4-Autopilot/src/modules/mc_pos_control/MulticopterPositionControl.cpp

   Lines 77 to 80 in 419652b

   	// clear update
   	parameter_update_s pupdate;
   	_parameter_update_sub.copy(&pupdate);

   
   It only got called once at boot and not anymore until I restarted the module using pxh> mc_pos_control stop pxh> mc_pos_control start.

To Reproduce

1. make px4_sitl sihsim_quadx

Expected behavior

Modules should update their parameters during runtime.

Screenshot / Media

No response

Flight Log

Software Version

Latest main, everything after 4a55393

Flight controller

v5x, SITL

Vehicle type

Multicopter

How are the different components wired up (including port information)

No response

Additional context

No response

[MaEtUgR]

Starnge that no one else found it before or did I just overlook it 👀 Could have to do with the timing at boot e.g. when modules subscribe to the parameter_update topic, no idea, just a guess.

[MaEtUgR]

I narrowed it down and the problem is "Added check to make sure hrt_elapsed_time can never be negative" here:
https://github.com/PX4/PX4-Autopilot/pull/22881/files#diff-5971d648b2b246dfb01ec5d5006b5258cfdbc41b73f50db1abe7dd5236f855e1R167-R172

I'm not sure why this was added nor why this should happen but I realized it happens all the time and the check changes the behavior significantly 👀

[MaEtUgR]

I did some more tests and at least in SITL some timestamps passed as *then argument are ~600ms before the timer started which means negative and in uint64_t values of for example 18446744073708979616. The calculation then wraps as expected and the subtraction results in the proper time difference. I don't know why this wouldn't be allowed. In theory the timestamp should also continue to work across a wrap although it's unlikely for PX4 to reach a runtime of 2^64 /10e6 / 60 / 60 /24 / 365 = 584'942 years.

Still would be interesting to see why some timestamps are before the timer starts counting 👀 . Would it be an option to allow negative time differences again?

[MaEtUgR]

Here would be that suggestion: #23380

[katzfey]

The reason that was added was that on VOXL 2 platform there are two time domains in PX4 which shift with respect to each other. So one side periodically gets a correction and it's timestamp will move. So it is possible that we get a negative value after such a shift and then that subtraction results in a huge positive value returned from hrt_elapsed_time. That causes things to run too early when it happens.

[katzfey]

If you want to represent "600ms before the start of time" then I would suggest making the input parameter a signed integer. Then you can do the math properly if it is in fact a negative number. If it is a positive number then return 0 if the "then" is greater than "now".

[MaEtUgR]

I have two improvement proposals:

1. Make sure there's no timestamp that wraps back into time before boot. I have the suspicion it comes from SubscriptionInterval because that's used for parameter updates with an interval of 1 second and the initial difference is exactly 1 second.
   
2. Not completely disallow time difference calculations across a wrap but rather disallow time differences of more than half of the 64-bit range which is 292k years.

[MaEtUgR]

suspicion it comes from SubscriptionInterval

Bingo, I'm getting there 👀

[MaEtUgR]

1. I have a fix here: Bugfix: Timestamp wrapping when initializing SubscriptionInterval less than the interval time after boot #23384
2. I'd have that ready on a branch here: https://github.com/PX4/PX4-Autopilot/compare/maetugr/wrap-detection-hrt_elapsed_time which I tested to work fine with timestamps that are e.g. 10 years before boot and still catches any wrapped timestamp in the future. But I still strongly suggest to not do this but rather revert to the efficient implementation like suggested in drv_hrt: allow time differences across timestamp wrap again #23380 I discussed this with @bkueng and the microcontroller timestamp must not jump backwards. This could also lead to problems in other parts of the system and guarding the wrapping case in hrt_elapsed_time() is a hack compared to adjusting the timestamp PX4 needs to be synced against e.g. the linux application.