fix: wrong TX polarity at CCPM pin for external PPM, DSM2 and LemonDSMP modules

[mha1]

Fixes #3413

Summary of changes:

• PPM: As neither g_model.moduleData[].ppm.pulsePol nor the yaml representation has changed over 2.8.1 I figured changing the logic at port initialization is the right place for PPM.
• DSM2 and LemonDSMP: changed initialization of serial port TX to normal

Tested on TX16s ok.

[mha1]

Notes for PPM:
As the meaning of polarities '-' and '+' are not really defined I'll do it here. This PR will result in PPM streams behave like in 2.8 with:

• PPM '-' -> idle high (= active low) CPPM stream
• PPM '+' -> idle low (= active high) CPPM stream

Notes for DSM2 and LemonDSMP:
Assuming 2.8 was correctly serving serial data to those external modules this PR changes 2.9 to the same behavior as in sending out idle high serial data.

[pfeerick]

Ok, I think this needs to be approached differently - it looks like the TX16S is the odd one out, not the reference radio in this case, as while it is fine for PPM, DSM2 and LemonDSMP, the X9D+2019, TX12MK2 and NV14 are all wrong for DSM2 and LemonDSMP now, but PPM is now right. 🤷

[mha1]

Do I understand you correctly this PR fixes PPM for all radios but serial for DSM2 and LemonDSMP is now correct for TX16s but wrong for the rest?

[pfeerick]

Yes, for the radios I tested, PPM (and PPM alone) was now correct.

[mha1]

ok, and with or without this PR DSM2 and LemonDSP are wrong for X9D+2019, TX12MK2 and NV14.

In other words: there was nothing wrong with DSM2 and LemonDSP for X9D+2019, TX12MK2 and NV14 in the first place? Only TX16s was wrong?

Sorry for the dumb questions but I only have my TX16s available and am trying to establish a baseline of what was wrong on which radio.

[mha1]

As a reference TX16s external module polarity at CCPM pin 2.8.1 vs 2.9 nightly without this PR:

Can you please let me know the 2.8.1 idle levels for:

[pfeerick]

Sometimes a picture is worth a thousand words (and helps to make sure the monkey behind the keyboard didn't goof up 🤭 )

When I refer to 2.9.0, this is with the currently nightly.

And the results are plain weird. As you can see, PPM is correct again, active neg. But for the EL18/NV14, this PR flipped DSM2 and DSMP. Although IIRC the EL18 has a programmable inverter, so that might need flipping also. But the X9D+2019 went wrong in main, and this PR didn't change that.

X9D+2019

EL18/NV14

[mha1]

@pfeerick Can you please repeat your tests, especially NV14 and X9D+. Trying a different approach, tested on my TX16s and - a round of applause please - thanks to @ParkerEde on X7access and X10epress.

[mha1]

@ParkerEde got his hands on a X9D+ (not 2019) and tested the latest commit. Everything as it should be.

[raphaelcoeffic]

@mha1 I'll try to have a look today. Some of the changes look fishy to me. I have the feeling this should be fixed differently, but I need to do a side-by-side comparison of the output signal (2.8 vs main).

[mha1]

@raphaelcoeffic I think one thing might be the lack of a clear definition for the term "inverted". For FrSky inverted was the normal. and inverting a signal from FrSky's perspective yields inverted output for FrSky but normal to the outside world. I'd rather talk about the quiescent (idle) or active level at the pin.

If we take quiescent level as the reference:

• idle level high is what most of the rest of the world would consider "normal" for serial communication over standard micro controller UART's
• idle level is the considered inverted

I think this is what makes the difference in the module/port API between using soft serial and UARTs. Soft serial outputs and expects idle high for polarity = normal. And I believe this is the other way around for the HW UART on external module bay due to FrSky's inverters. It'd be worth to harmonize this. I believe the only way to get this sorted is to have a clear definition of what normal and inverted means for the levels at the pins.

[mha1]

@raphaelcoeffic Oh, and there is probably one change in radio/src/targets/common/arm/stm32/module_timer_driver.cpp you might ask why it's done.

Turning on PPM by going from OFF to PPM in external modules produces two back to back frames which might confuse some modules. This is the very first that's output after selecting PPM. You can see the back to back frames:

Same sequence after the change:

[raphaelcoeffic]

@mha1 the intent is the following:

• normal means idle-high for serial (MCU output).
• for radios that can control output inverters, the semantic is aligned to those which don't have them (for historical reasons).

As for what is what in terms of PPM output, the same parameters should lead to the same output in 2.8 and main.

And yes, I'm wondering why we should de-init the DMA on each pulse sending. I do have some changes locally to init the DMA only once at setup, and then only enable/disable the stream. That allows for more flexibility for driver users that need different settings (WS2812 LEDs, for instance).

[mha1]

@mha1 the intent is the following:

* `normal` means **idle-high** for serial (MCU output).

* for radios that can control output inverters, the semantic is aligned to those which don't have them (for historical reasons).

This is what I mean. It's confusing what happens at the module bay pin. My humble opinion: the module/serial API is an abstraction layer that hides various types of hw designs. It should take care of business end-to-end. No thinking about hw design, no thinking about inverters. Think only in terms of what kind of protocol (logical and electrical) is sent/expected at the module bay's pins.

As for what is what in terms of PPM output, the same parameters should lead to the same output in 2.8 and main.

Correct. Status 2.9 vs 2.8: Levels for PPM are wrong way around, idle high serial output (as used by DSM2 and LemonDSMP) is wrong too for Horus and Taranis targets but are correct for NV14. I still believe this is a side effect of looking at MCU outputs rather than end-to-end. Therefore the port specific polarity settings in this PR. inverted for ETX_MOD_PORT_UART, normal for ETX_MOD_PORT_SOFT_INV to hopefully achieve the same result (waiting for NV14 test).

And yes, I'm wondering why we should de-init the DMA on each pulse sending. I do have some changes locally to init the DMA only once at setup, and then only enable/disable the stream. That allows for more flexibility for driver users that need different settings (WS2812 LEDs, for instance).

Maybe we should separate the two topics. The de-init I added can be removed again as it probably doesn't hurt having two back-to-back frames at startup of PPM. It just doesn't look right. So does the very first frame btw. It's an all low frame. I couldn't identify why this happens so far.

[mha1]

converted to draft in favor of #3513

[pfeerick]

You're not getting off that easily... it might be two weeks later, but here are the results for the EL18 (basically the same as the NV14 for these tests)... which do seem to match 2.8.1. Now to try out #3513...

PPM_neg

DSM2

DSMP

[mha1]

Well, it seems I got off the hook, doesn't it? I checked #3513 a few days ago. It solved the problems on my TX16s but I found another issue with the transition from OFF to PPM. I also asked Raphael to check out NV14 and X9D to cover at least some significant hardware variants. He fixed the OFF to PPM On transition and with your feedback on #3513 TX16s, NV14 and X9D looked good. I'm glad Raphael fixed it himself because it's his code. That's the way it should be.

PS - my counter to getting off easily:

• Thanks for merging fix: LUA ignoring GVAR precision #3540 and fix(color) - Update GV value in header when precision changes. #3544#
• Talked with Neil, fix: single and updated source of Multi protocol definitions #3435 is ready to go and should receive community testing.
• Now that fix: signal polarity for soft-serial & timer based protocols #3513 is in main all prerequisites for Support for external Multiplex MLink JR type module #3352 are fulfilled. Force pushed the PR, tested ok. If you want to give it a go use my simple MLink module telemetry simulator you already used some time ago and https://github.com/EdgeTX/edgetx/actions/runs/4893474159?pr=3352
• Please have a look at RX stat labeling dependent on telemetry protocol  #3523. I think it is worth while for 2.9 and can be extended very easy. But I agree it should be replaced in the course of the big telemetry overhaul in the (far?) future. Needs merge of add setters for DynamicNumber prefix and suffix texts libopenui#90
• I'd have more ...

[gagarinlg]

* I'd have more ...

next dev discussion hour on discord is on the 7th of Mai ,9am UTC / 11 am CEST
https://discord.com/channels/839849772864503828/1081860475781914624

[gagarinlg]

replaced by #3513