Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prioritize module with higher update rates when internal and external module are active #2920

Closed
wants to merge 22 commits into from

Conversation

mha1
Copy link
Contributor

@mha1 mha1 commented Dec 21, 2022

This implements enhancement proposal #2919

Summary of changes:

If both modules are active the one with the higher update rate drives the mixer task and hence also receives the perk of being synced. The slower module will be updated at an integer fraction (divider) of the higher rate module. The divider is calculated such that the resulting update rate for the slower module will be as close as possible to its requested rate but doesn’t exceed it. It doesn’t matter if the higher update rate module is internal or external. The higher update rate module will always drive the mixer task, the slower one will follow.

Example: TX16s external ELSR and internal MPM (requests 7000us update period):
500Hz ELRS /143Hz MPM-> 500Hz sync/125Hz no sync (divider 4)
333Hz ELRS/143Hz MPM -> 333Hz sync/111Hz no sync (divider 3)
100Hz ELRS/143Hz MPM -> 71Hz no sync (divider 2)/143Hz sync

Telemetry data of both modules will be received and is discoverable.

This PR also contains a change to the Radio Version Modules/RX version pop-up to show the real update rates for the internal and external module.

Current implementation showing internal MPM driving the mixer schedule period and forcing the external module to the same update rate while the external module is set to 500Hz. Internal module is master:
screen-2022-12-21-112739

New implementation with ELRS set at 500Hz and MPM active. ELRS is master:
screen-2022-12-21-111954

New implementation with ELRS set at 333Hz and MPM active. ELRS is master:
screen-2022-12-21-112041

New implementation with ELRS set at 250Hz and MPM active. ELRS is master:
screen-2022-12-21-112116

New implementation with ELRS set at 150Hz and MPM active. ELRS is master:
screen-2022-12-21-112143

New implementation with ELRS set at 100Hz and MPM active. MPM is master:
screen-2022-12-21-112221

New implementation with ELRS set at 50Hz and MPM active. MPM is master:
screen-2022-12-21-112311

Copy link
Member

@raphaelcoeffic raphaelcoeffic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In preparation of coming changes, I'd strongly prefer these functions to use a module ID (index) rather than explicit names. The reason is that I will start soon to make the module interface as generic as possible, without functions called internal or external, as it basically forces us to double the code or add conditional closes where not necessary.

Talk is cheap, let's show some code :-) This is the kind of API I'm thinking of:

// Fetch the module index of the module responsible for synchro
uint8_t mixerSchedulerGetSyncedModule();

// Fetch the real scheduling period of a given module
uint16_t mixerSchedulerGetPeriod(uint8_t moduleIdx);

@mha1
Copy link
Contributor Author

mha1 commented Dec 21, 2022

@raphaelcoeffic

Sanity check to see if I understood you right.

Changing this

// Fetch the real scheduling period internal module
uint16_t getRealMixerSchedulerPeriodInternal();

// Fetch the real scheduling period external module
uint16_t getRealMixerSchedulerPeriodExternal();

to

// Fetch the real scheduling period of a given module
uint16_t mixerSchedulerGetPeriod(uint8_t moduleIdx);

The calls will be mixerSchedulerGetPeriod(INTERNAL_MODULE) and mixerSchedulerGetPeriod(EXTERNAL_MODULE), right?

Same for

// check if internal or extern module is synced
bool isSyncedModuleInternal();

to
uint8_t mixerSchedulerGetSyncedModule();

This will return either INTERNAL_MODULE or EXTERNAL_MODULE, correct?

@mha1
Copy link
Contributor Author

mha1 commented Dec 21, 2022

@raphaelcoeffic hope this is what you had in mind

pfeerick and others added 9 commits December 22, 2022 10:52
* Added EL18 specific firmware naming.

* fix: Update USB identifier

* chore: Add to github builds

* fix: Lowercase s in Flysky

* chore: Reference EL18 specific firmware file

Co-authored-by: Peter Feerick <[email protected]>
the module with the slower update rate follows the faster one (which gets the sync perk). The slower module will be updated at the closed integer dividable rate possible of the faster one without exceeding the max rate of the faster one.

500Hz ext, 143Hz int -> int will get 125Hz (divider 4)
333Hz ext, 143Hz int -> int will get 111Hz (divider 3)
250Hz ext, 143Hz int -> int will get 125Hz (divider 2)
150Hz ext, 143Hz int -> int will get 75Hz   (divider 2)
100Hz ext, 143Hz int -> ext will get 71Hz  (divider 2)
@raphaelcoeffic
Copy link
Member

raphaelcoeffic commented Dec 22, 2022

@mha1 I rebased the code on main (new features are always on main first) and generalised a fair bit. To avoid issues, you should use the Github CLI to force using the latest state (see here) with the following:

gh pr checkout 2920 --force --recurse-submodules

However, I think there are still a couple of issues that need to be solved:

  • when modules have frequencies in the same order of magnitude, the module that does not get the sync will be refreshed much less often.
  • FrSky internal modules do not use timers for the sync, they use a signal on Heartbeat pin. The scheduler timer is set to a safe value approx. 2ms above what is expected. We will need a solution for this as well.
  • I think we should not be using getMixerSchedulerPeriod() in the UI code, as it effectively modifies parameters used by the sync. See gui/128x64/model_setup.cpp, gui/common/stdlcd/radio_version.cpp, gui/212x64/model_setup.cpp, and gui/colorlcd/crossfire_settings.cpp.

@mha1
Copy link
Contributor Author

mha1 commented Dec 22, 2022

  1. Yes, this is true and can be seen in the ELRS 150Hz / MPM 143Hz case. One way to solve this could be to unlink the pulse generation from the mixer task for the slower module, i.e. have a separate task (software timer?) running at the unsynced rate that feeds off the mixer task results. As the mixer task runs at a higher priority this should even be safe.

  2. Dang, FrSky. Haven't looked at this at all. Can you point me to where the heart beat is generated?

  3. This was always the case as the period fed off mixerSchedules[].period which in the synced case will be constantly adjusted, won't it? But to follow your API guideline the call should be getMixerSchedulerRealPeriod(uint8_t moduleIdx).

@mha1
Copy link
Contributor Author

mha1 commented Dec 23, 2022

@raphaelcoeffic

Here's a wild idea to improve 1: Run mixer task at double the masters sync frequency and have the pulse generation run at divider 2 for the master and calculate the slave divider based on the doubled mixer task frequency. doMixerCalculations() and doMixerPeriodicUpdates() can run on alternating cycles to distribute CPU load evenly.

Here's a comparison of running the mixer task at the sync frequency to running the mixer task running at double the sync frequency. Check out the slave frequency deviations wrt to their requested frequency. This could be extended to 4 times the sync frequency with even better results, but a less even CPU load and slightly more CPU overhead.

image

@gagarinlg
Copy link
Member

With ELRS running at 1 kHz this won't work. There is not enough computing time.

@mha1
Copy link
Contributor Author

mha1 commented Dec 23, 2022

With ELRS running at 1 kHz this won't work. There is not enough computing time.

There is no (well a little) additional CPU load as the mixer tasks computation would be distributed over two mixer task cycles. Double the frequency, half the work per cycle.

@gagarinlg
Copy link
Member

I don't think that this will work as intended. What is your idea on how to split the work between the runs?

@mha1
Copy link
Contributor Author

mha1 commented Dec 23, 2022

roughly:

cycle odd: doMixerCalculations() and setupPulses(syncedMaster)
cycle even: doMixerPeriodicals()
And at cycle modulo dividerSlave == 0: setupPulses(unsyncedSlave)

The doubling of the frequency merely serves to have a finer resolution for the unsynced slave freq generation, see calculation above. The mixer task running at 2kHz will still result in a 1kHz completion of the mixer computation and pulse generation for the synced module (which asked for 1kHz).

…e generation run at divider 2 for the master and calculate the slave divider based on the doubled mixer task frequency. doMixerCalculations() and doMixerPeriodicUpdates() run on alternating cycles to distribute CPU load evenly.
@mha1
Copy link
Contributor Author

mha1 commented Dec 24, 2022

Implemented idea to improve slave frequency deviation.

Example ELRS set to F1000, 961KBaud and MPM as slave at 143 Hz. Mixer task running at 1000Hz*2 = 2000Hz with max duration for simple mixer setup still around 0.3ms.

image

Mixer task at 1000Hz*2=2000Hz. Slave at 2000/14 = 142,8 Hz, Master at 2000/2 = 1000Hz.
image

Example 333Hz/143Hz: Mixer task at 666Hz, Slave running at 666/5 = 133,2Hz, Master at 666/2 = 333Hz
image

@mha1 mha1 marked this pull request as draft December 24, 2022 16:33
tested ok with using ELRS external module/receiver and MPM/Graupner HoTT receiver
@raphaelcoeffic
Copy link
Member

raphaelcoeffic commented Dec 25, 2022

  1. Yes, this is true and can be seen in the ELRS 150Hz / MPM 143Hz case. One way to solve this could be to unlink the pulse generation from the mixer task for the slower module, i.e. have a separate task (software timer?) running at the unsynced rate that feeds off the mixer task results. As the mixer task runs at a higher priority this should even be safe.

I was thinking we could have something like:

  • sync on the fastest module as you propose,
  • but send the pulses for the other modules at the same pace, if it is not faster as a protocol specific maximum rate (to be defined).
  • if the sync is faster than the maximum rate, use a multiple of the synced rate (as you implemented) to have the pulses sent to that module at a rate that is below the maximum rate.

As an example:

  • ELRS at 1 kHz,
  • MPM 143 Hz (max rate 300 Hz)

In that case a multiplier of 4 (instead of 8 for the current implementation) would be chosen.

The reason for this is that most modules should be able to tolerate a higher rate than nominal, but might have troubles with a rate that is lower than the nominal rate.

For instance, FrSky XJT will start doing strange things when the rate is lower than its nominal rate. Others might fall into failsafe mode, etc.

  1. Dang, FrSky. Haven't looked at this at all. Can you point me to where the heart beat is generated?

The heartbeat driver is here:
https://github.com/EdgeTX/edgetx/blob/main/radio/src/targets/common/arm/stm32/heartbeat_driver.cpp

It is activated for instance here:

static void* pxx1InitInternal(uint8_t module)
{
void* uart_ctx = IntmoduleSerialDriver.init(&pxx1SerialInit);
#if defined(INTMODULE_HEARTBEAT)
init_intmodule_heartbeat();
#endif
mixerSchedulerSetPeriod(INTERNAL_MODULE, INTMODULE_PXX1_SERIAL_PERIOD);
INTERNAL_MODULE_ON();
return uart_ctx;
}

here you can see that when the heartbeat sync is used, the timer is set with an additional 1ms, but we expect the heartbeat to actually trigger earlier:

#if defined(INTMODULE_HEARTBEAT)
// use backup trigger (1 ms later)
#define INTMODULE_PXX1_SERIAL_PERIOD (9000 + 1000)/*us*/
#else
#define INTMODULE_PXX1_SERIAL_PERIOD 9000/*us*/
#endif

  1. This was always the case as the period fed off mixerSchedules[].period which in the synced case will be constantly adjusted, won't it? But to follow your API guideline the call should be getMixerSchedulerRealPeriod(uint8_t moduleIdx).

Yes, I believe this it.

@raphaelcoeffic
Copy link
Member

raphaelcoeffic commented Dec 25, 2022

Regarding the heartbeat sync (FrSky modules), I believe we could use it a little differently to better adjust for your proposal.

Right now the heartbeat driver directly triggers the mixer, which does not pass the real the rate/frequency to the scheduler. We could however decouple the heartbeat trigger from the mixer schedule, and use it only as a feedback mechanism to compute rate/phase, as provided directly by other modules via telemetry feedback.

That would imply computing a corrected rate / delay when the heartbeat ISR is triggered, and feeding this via mixerSchedulerSetPeriod to the scheduler. The mixer would be solely triggered by the scheduler, and not by the heartbeat trigger anymore.

What do you think?

@mha1
Copy link
Contributor Author

mha1 commented Dec 25, 2022

I was thinking we could have something like:

* sync on the fastest module as you propose,

* but send the pulses for the other modules at the same pace, if it is not faster as a protocol specific maximum rate (to be defined).

* if the sync is faster than the maximum rate, use a multiple of the synced rate (as you implemented) to have the pulses sent to that module at a rate that is bellow the maximum rate.

Hi Raphael,

If you assume there will be problems with updating (some) modules at lower than requested rates your last bullet point shows this is not a solution as there will always be combinations where master freq is higher than requested slave frequencies.

If your assumption is correct the concept of following the master freq failed. My assumption was it is ok to serve updates at up to the requested rate, just avoiding to flood the module. If this is correct my thinking was to minimize the deviation real vs requested rate by finding ways to increase the divider resolution. This is the reason why I experimented with doubling the mixer task frequency as this increases the divider resolution, see Excel sheet. It's working. The slave will for most combinations be updated closer to the requested rate but still at lower rates than requested.

In general we have to consider three requirements:

  1. being in sync with the mixer calculations (i.e. use mixer result with the minimal possible latency)
  2. running the mixer calculations at the requested rate
  3. adjusting the mixer calculation timing to the needs of the module

The concept of selecting the faster module to drive the mixer task fulfills 1., 2., 3. for the faster module. Checked.

Now for the slower module. If your assumption is correct we need to sacrifice at least one of the three requirements as 2. would then be mandatory. We could ensure 2. by making the slower freq generation independent of the master freq, i.e. have a totally asynchronous slave freq generation. This will sacrifice 1. and possibly allow to implement 3. The way to implement this could be a software task running at the requested rate (maybe adjusted with lag data of the slower module). The downside by having to scrap 3 is additional latency jitter for the slower module. What do you think?

For FrSky: I think this is the way to go. Sort of unifying the module updating mechanisms.

Merry Christmas - Michael

@rotorman
Copy link
Member

How about following, have to admit a bit wild, idea: all STM32F2/F4 microcontrollers used in presently supported radios come with 3 internal ADCs. How about dedicating one ADC for the internal module, other for external module and letting them run, incl. the mixer tasks (plural here!) at their native paces?
This solution does not solve the issue for 3 cases though: MCU external ADCs (e.g. X12s) and PWM or UART based gimbals, but would allow us to go with two separate native frequencies for the radios with analog gimbals and STM32 internal ADC sampling.

@mha1
Copy link
Contributor Author

mha1 commented Dec 25, 2022

How about following, have to admit a bit wild, idea: all STM32F2/F4 microcontrollers used in presently supported radios come with 3 internal ADCs. How about dedicating one ADC for the internal module, other for external module and letting them run, incl. the mixer tasks (plural here!) at their native paces? This solution does not solve the issue for 3 cases though: MCU external ADCs (e.g. X12s) and PWM or UART based gimbals, but would allow us to go with two separate native frequencies for the radios with analog gimbals and STM32 internal ADC sampling.

Hi Risto,

Merry Christmas to you!

Thanks for chiming in. No, not that bad of an idea. I actually thought about this too without going into details to deep. My worry was two mixer tasks and pulse generations running could overwhelm the CPU. A quick check with my most complex mixer setup (6 control surface glider with rc-soar like mixer cascading) had a worst case of > 1ms mixer task duration (Tmix max in Statistics/Debug) with rapid movement of the sticks while fast switching between flight modes setup with transition time. I just did it again to take a screenshot and saw 1.36ms Tmin max. The theoretical worst case for the mixer task is ELRS set at 1kHz packet rate (nonsense but no stopping users to do it). 1.36ms is most likely a very rare peak with averages well under 1ms but shows there is not a lot of wiggle room if you want tomaintain a stable 1ms mixer task schedule required for 1kHz packet rate. I decided doubling the computational requirements will not work reliably. Maybe I was to quick but going for the idea will need a close eye on CPU load. Can you verify my observations and conclusion? I think best would be to setup timing measurements for the mixer tasks and lower periodic task and see if they meet their deadlines under stress conditions.

Michael

IMG_5256

PS: I measured the menus task (50ms period) some time ago with 2.7 and a non-trivial model setup idling but with active logging. Measurement showed menus task completion times varying from 15-35ms meaning roughly 25%-30% reserve. Not an awful lot.

menusTask_period
menusTask_duty_low
menusTask_duty_high

@mha1
Copy link
Contributor Author

mha1 commented Dec 26, 2022

@raphaelcoeffic

Regarding the heartbeat sync (FrSky modules)

Here's my first attempt at this. Changes made to

  • heartbeat_driver.cpp
  • pxx1.cpp
  • pxx2.cpp
  • radio_version.cpp (for debugging)

The heart beat driver calculates the time difference between the last and the actual heart beat using the 2MHz timer. The period and the last time a heart beat was received is fed to the module sync status (zero lag). The pulse generation routines for PXX1 and PXX2 (both internal and external) check if valid heart beats were detected (200ms timeout, call to isValid() of module sync status). If not the default period is used, otherwise the mixer scheduler period is set to the adjusted value. The adjusted value is used to set the mixer timer interrupt and to calculate the master period/divider and slave period/divider. I have no hardware to test this. Can you please build this branch and give it a try?

This is from simu showing (9000+1000)us because there are no valid heart beats. If running on real hardware this should show 9000 or 4000 depending on the module.
image

PS: unit tests will fail due to doubling the mixer task frequency change. Test file needs updates. Will do if change proves useful.

@mha1
Copy link
Contributor Author

mha1 commented Dec 30, 2022

Here's some timing measurements of the mixer task with ELRS running at the worst case scenario 1000Hz ELRS and MPM requesting 143Hz and a model with my most complex rc-soar like 6 control surfaces glider setup. This setup was tested successfully with BetaFPV 2.4 (and 868) module/receiver and MPM running Graupner HoTT with Grupner HoTT receiver.

The 1000Hz/143Hz combination will result in the mixer task running at 2kHz with a synced ELRS pulse generation at 1000Hz (divider 2) and the MPM running at 142 Hz (divider 14).

The total mixer task processing is spread over two cycles, one even, one odd cycle. At every even cycle the mixer calculations and synchronous pulse generation are performed. This takes about 455us with the system idling, i.e. no flight mode transitions, no stick movement, etc.

It is worth to note here that this duration is independent of the mixer frequency. So running a 1kHz update rate in combination with a complex mixer setup even without a second module active will at least sporadically overwhelm the CPU and will lead to other tasks suffering with 2.8 release firmware. 1kHz needs to be considered as a "you know what you are doing" mode and will only provide reliable and constant latency results in combination with low complexity mixer setups. Tmin Max in Statistics is a good indicator. It should stay well under 0.6ms to have a reasonable CPU margin left for other tasks.

Back to this PR. Here's a screenshot showing the two cycle strategy and the mixer calculations and synchronous pulse generation using above described setup:
1

A closer look at the even cycle:
2

This shows the frequency of the asynchronous pulse generation. With the above mentioned setup (divider 14) 142Hz (7000us period) are expected.
5

Looking closer at the penalty this PR adds. It's about 700ns in every even and odd cycle to decide if it's time to generate asynchronous pulses.
3

Here's an example where asynchronous pulses need to be generated in an odd cycle. In total having a second module active adds about 20us max penalty.
4

Takeaways:

  • 1kHz should generally be run only in combination with low complexity mixer setups independent of this PR. The CPU is at it's limits.
  • The majority of processing time is consumed be the mixer calculations. The "double the mixer frequency to allow higher resolution for the asynchronous pulse generation" strategy works with fairly low additional computational effort.

Open points:

  • testing of heart beat driven mixer scheduling. I don't have hardware for this

If you are interested in taking a closer look at the measurements:

@mha1
Copy link
Contributor Author

mha1 commented Dec 30, 2022

I did some reference measurements using 2.8 release to check this PR doesn't add extraordinary CPU load and also to emphasize my point about 1kHz might be right on the edge if not over the top of what the CPU can handle. Again nearly the same setup as above external ELRS at 1kHz update rate, internal module OFF with my rather complex 6 surface glider setup.

In 2.8 the mixer task runs at the to be synced frequency and mainly performs three calculations in sequence. First the mixer calculations, then the pulse generation followed by the period mixer updates. With the system idling we see a period of 1000us and roughly the same duration for the calculations. The mixer calculation consumes about 450us:
1a

The pulse generation consumes roughly 16us:
2a

And the periodic mixer updates consume around 500ns:
3a

With putting more stress on the system by switching flight modes Tmix max exceeds 1ms. Remember, about 1ms is the total CPU time the mixer task can consume without missing its deadline (latency no longer gauranteed) BUT is then already starving all other tasks (like UI, audio, LUA) in the system.
4a

Tmix max shows the maximum peak which is very likely not happening very often, but means overall CPU load is too high. Here's a measurement taken at a random point in time while the flight mode transition is in progress showing about 700us for the mixer calculation. This pushes the mixer task to about 75% CPU load. Not a lot of CPU left to feed the other tasks which is noticeable even for the user. I have audio output announcing the new flight mode which begins to stutter.
6a

Summary:

  • 1kHz pushes the limits
  • Carefully observe Tmin max. I'd say everything above 0.8ms is too high to run 1kHz
  • Avoid complex mixer setups at high rates
  • CPU load must be considered when further extending functionality

Any thoughts?

@mha1
Copy link
Contributor Author

mha1 commented Mar 16, 2023

nuked by the serial/module API renovation

# Conflicts:
#	radio/src/gui/colorlcd/crossfire_settings.cpp
#	radio/src/gui/colorlcd/radio_version.cpp
#	radio/src/gui/common/stdlcd/radio_version.cpp
#	radio/src/mixer_scheduler.cpp
#	radio/src/mixer_scheduler.h
#	radio/src/tasks/mixer_task.cpp
#	radio/src/tests/mixer_scheduler.cpp
@mha1
Copy link
Contributor Author

mha1 commented Jun 1, 2023

closed in favor of #3642

@mha1 mha1 closed this Jun 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants