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

refactor: notify module driver on config changes #3780

Merged
merged 5 commits into from
Oct 1, 2023
Merged

Conversation

raphaelcoeffic
Copy link
Member

@raphaelcoeffic raphaelcoeffic commented Jul 8, 2023

In order to remove certain limitations and thus further generalise the modules' life cycle, it is necessary to notify the module driver on certain config changes in order to let it decide what is required to apply these changes.

@mha1
Copy link
Contributor

mha1 commented Jul 8, 2023

doesn't work - requires logic to reinitialize module after subtype change

@raphaelcoeffic
Copy link
Member Author

raphaelcoeffic commented Jul 9, 2023

requires logic to reinitialize module after subtype change

I think that was @gagarinlg 's point: which changes exactly need a restart should be decided by the module driver.

I have the feeling this PR will grow and grow....

@raphaelcoeffic raphaelcoeffic changed the title chores(ppm): simplify telemetry init refactor: notify module driver on config changes Jul 9, 2023
@gagarinlg
Copy link
Member

Which is not necessarily a bad thing. When it makes the code better and prepares it to be multi-Cpu capable, I am happy that we have a starting point.

@mha1
Copy link
Contributor

mha1 commented Jul 9, 2023

last but one commit sent TX16s into a boot loop

last commit works for switching between PPM/No Telemetry and PPM/Mlink. Mlink is properly initialized after switch to MLink. Also ok at power up with PPM/MLink set.

@raphaelcoeffic
Copy link
Member Author

@mha1 nice! Do you imply I should start testing the code? ATM I was more looking for "how would it look like" rather than 100% running code.

I will try to hunt for more hacks that could be replaced so we move more stuff into module drivers that actually belong there.

@gagarinlg
Copy link
Member

👍

@raphaelcoeffic
Copy link
Member Author

@mha1 regarding the telemetry init, I think there is something strange.

Did you really mean to use the external module bay UART RX pin (ETX_MOD_PORT_UART)? It is only present in certain radios, mostly ACCESS compatible and Flysky NV14/EL18. On others it is the "heartbeat" pin. Or did you mean actually S.PORT in both cases?

  if (modulePortInitSerial(module, ETX_MOD_PORT_UART, &ppmMLinkSerialParams) != nullptr) {
    return true;
  }

  if (modulePortInitSerial(module, ETX_MOD_PORT_SPORT_INV, &ppmMLinkSerialParams) != nullptr) {
    return true;
  }

In case you didn't know, some radios have a hardware inverter on S.PORT (Flysky NV14/EL18), so that you might want to probe this as well (ETX_MOD_PORT_SPORT with polarity == ETX_Pol_Inverted).

@mha1
Copy link
Contributor

mha1 commented Jul 16, 2023

Hi Raphael, good catch and glad you're asking. I am aware of the NV14 software controlled inverters and intended to probe this one first, then if failing, probe soft serial, both with ETX_Pol_Inverted. Of course it should read ETX_MOD_PORT_SPORT to cover the NV14/EL18.

The only excuse I have is at the time of implementing this I wasn't absolutely sure which ETX_MOD_PORT ultimately ends up at which module bay pin. This was the reason I asked you for assistance on March 19th (check your DM's) to figure this out . With no documentation and a rough idea by reverse engineering the code I checked if other protocols had the same requirement "active low serial RX on the SPort pin" and found DSM2. Unfortunately dummy me looked at wrong initialization. I copied the TX init instead of the RX init. That's why it reads ETX_MOD_PORT_UART. Still working on all radios with module bay apart NV14/EL18 so I (and testers) never noticed.

Please allow me to repeat my call for some documentation on ETX_MOD_PORTs. ETX_MOD_PORT_xxx --- module bay (Pin on different radio clusters) --- ETX_Pol_Normal/Inverted available --- real polarity at that pin (active low, high) with ETX_Pol_Normal/Inverted.

@mha1
Copy link
Contributor

mha1 commented Jul 16, 2023

Shouldn't the module port initialization not be a lot more abstract (i.e. simpler) on the API level? The application should just request a port (logical pin, e.g. module bay SPORT) with certain parameters and let init be kind of a broker managing hw dependencies, deciding if appropriate hw is available and if so processing the necessary low level initialization like configuring inverters or choose a different path like soft serial. I mean why should a telemetry driver know about hardware inverters. Why the trial and error approach.

In the above case I'd only request an active low 38400N1 serial connection at the module bay SPORT known per convention to be the first pin of the 5 pin external module which is defined to be GPIO x on one and GPIO y on another radio, Init knows if we are running on an NV14 or whatever radio and would have made the correct connection, i.e. soft serial on TX16s or serial with turned off hardware inverters on NV14.

@gagarinlg
Copy link
Member

I agree

@raphaelcoeffic
Copy link
Member Author

raphaelcoeffic commented Jul 16, 2023

The application should just request a port (logical pin, e.g. module bay SPORT) with certain parameters and let init be kind of a broker managing hw dependencies

That is already the case for most things. What is missing really is the automatic fallback to soft-serial. The reason was that we might have different baud rates in that case. This is still the case for the Flysky FRM303 module (see AFHDS3 driver). So we would need to at least be able to block the automatic fallback and request specifically for HW-bound port or soft-serial.

So basically this would be about automatic fall-back between (only with ETX_Pol_Inverted):

  • ETX_MOD_PORT_UART -> ETX_MOD_PORT_SOFT_INV,
  • and ETX_MOD_PORT_SPORT -> ETX_MOD_PORT_SPORT_INV.

@mha1
Copy link
Contributor

mha1 commented Jul 16, 2023

// auto mod_st = modulePortInitSerial(module, ETX_MOD_PORT_UART, &sbusUartParams);
// TODO: check if inverter is there, or mandate it somehow...
// if (!mod_st) {
// -> SOFT_INV has 'setPolarity'

job for the broker

@mha1
Copy link
Contributor

mha1 commented Jul 16, 2023

quick check for ETX_MOD_PORT_SOFT_INV and ETX_MOD_PORT_SPORT_INV: no application changes baud rates if previous ETX_MOD_PORT_UART or ETX_MOD_PORT_SPORT was not successful

@gagarinlg
Copy link
Member

UI has to ask the HW what speeds are available for a selected module type.
(via a messaging interface between UI and HW)

@raphaelcoeffic
Copy link
Member Author

@mha1 the SBUS code needs to be "finished". It seems it was modified last as the module_port HAL driver was not finished yet.

@raphaelcoeffic
Copy link
Member Author

UI has to ask the HW what speeds are available for a selected module type.

Please no! We know very well that soft-serial needs a lower baud rate and is supported this way in the AFHDS3 driver, so no need for another setting here!

@raphaelcoeffic
Copy link
Member Author

I can have a try at automatic fallback when I come back from my vacations, I'll put it on my TODO list.

@mha1
Copy link
Contributor

mha1 commented Jul 16, 2023

Thanks, have a nice and relaxing vacation.

Let me summarize your ToDo list:

  • ✅ return home safely
  • ✅ not so many other important things
  • 👻 module port documentation for Michael
  • 👻 implement serial rx callback
  • 👻 make CLI work
  • ✅ module port automatic fallback (see feat: Automatic soft-serial fallback for module ports #3823)
  • 🤞 review Michael's LUA volume override PR
  • ✅ lots of not so important things

Let me know if I can be of any help.

@raphaelcoeffic
Copy link
Member Author

I can have a try at automatic fallback when I come back from my vacations, I'll put it on my TODO list.

See #3823 (completely untested)

@raphaelcoeffic
Copy link
Member Author

@mha1 if you're ok with this PR, I'd pass it on to @pfeerick for further testing.

@mha1
Copy link
Contributor

mha1 commented Aug 12, 2023

@raphaelcoeffic I'm good but was only testing PPM re-initialization after telemetry type change
@pfeerick give it a good run for the money

@pfeerick pfeerick added this to the 2.10 milestone Sep 29, 2023
@pfeerick pfeerick added the house keeping 🧹 Cleanup of code and house keeping label Sep 29, 2023
@pfeerick
Copy link
Member

pfeerick commented Oct 1, 2023

Nothing jumped out walking through internal and a couple modules (GHST, PPM OrangeRX DSM, Tracer) on TX16S and X9D+2019... each connected to respective receiver and seemed to work just fine.

@pfeerick pfeerick merged commit 00cb9c4 into main Oct 1, 2023
37 checks passed
@pfeerick pfeerick deleted the ppm-telemetry branch October 1, 2023 05:19
pfeerick added a commit that referenced this pull request Oct 1, 2023
Forgot to push before merge
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
house keeping 🧹 Cleanup of code and house keeping
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants