EdgeTX / edgetx

EdgeTX is the cutting edge open source firmware for your R/C radio
https://edgetx.org
GNU General Public License v2.0
1.54k stars 329 forks source link

Module / RX version pop up doesn't look nice #2320

Closed mha1 closed 1 year ago

mha1 commented 1 year ago

Is there an existing issue for this problem?

What part of EdgeTX is the focus of this bug?

Transmitter firmware

Current Behavior

"Multimodule" shouldn't be wrapped like that.

image

Expected Behavior

no breaking words

Steps To Reproduce

select Radio Settings, Version, Models / RX version

Version

Nightly (Please give date/commit below)

Transmitter

Radiomaster TX16S

Anything else?

build 9fbf0b7b

pfeerick commented 1 year ago

I thought this was mentioned in another issue recently, as I commented it's even worse on the NV14 due to the portrait screen... basically is a table with three columns, so is wrapping. It may be better to change it to two colums, and have a second line. At the moment it says Module (but I think it could also say "Receiver" in some instances). So instead it could be

Module: Multimodule
Status: No MULTI_TELEMETRY detected

Even if it that wrapped (i.e. on the NV14) it should look a lot better.

Anyway, the version dialog is here => radio/src/gui/colorlcd/radio_version.cpp

mha1 commented 1 year ago

I changed the popup to two columns and two separate lines for type and status.

Here's a picture to show the full scrollable layout (receiver made temporarily visible):

image image

Internal module MPM, external module OFF and receiver made hidden again (depends on hardware presence):

image

Internal MPM, external module CSRF:

image

Internal module OFF, external module FLYSKY:

image

Concerning the "no information" external modules I found no code that outputs the name of the external module for modules other than MPM, CSRF and PXX2. One improvement could be to use g_model.moduleData[module].type to show at least the name of the enabled (model settings) external module, e.g. one of those:

  MODULE_TYPE_PPM,
  MODULE_TYPE_XJT_PXX1,
  MODULE_TYPE_DSM2,
  MODULE_TYPE_R9M_PXX1,  // R9M
  MODULE_TYPE_R9M_LITE_PXX1,  //R9MLite
  MODULE_TYPE_R9M_LITE_PXX2,  //R9MLP                
  MODULE_TYPE_GHOST,
  MODULE_TYPE_R9M_LITE_PRO_PXX2,
  MODULE_TYPE_SBUS,
  MODULE_TYPE_FLYSKY,
  MODULE_TYPE_LEMON_DSMP,

Btw: do R9M_LITE_PXX2 and R9M_LITE_PRO_PXX2 not count as PXX2?

inline bool isModulePXX2(uint8_t idx)
{
  return isModuleISRM(idx) || isModuleR9MAccess(idx) || isModuleXJTLite(idx);
}
gagarinlg commented 1 year ago

Can you please test the NV14/EL18 via the simulator or simu?

mha1 commented 1 year ago

NV14 internal module FLYSKY, external module CSRF. Don't really know why it breaks "no information". But if we follow my proposal we would show "FLYSKY"

image

mha1 commented 1 year ago

Sorry again. Me dummy forgot to build for the NV14. I'll try again

mha1 commented 1 year ago

Please take the picture above as NV14 'before'. Here's the NV14 'after' with internal FLYSKY and external MPM.

image

and with internal FLYSKY and external CSRF

image

And it would be easy to get rid of the "no information" and replace it with the name of the protocol. I'd extend radio/src/gui/colorlcd/radio_version.cpp to have a switch/case on the selected module and just hardcode outputting the name instead of -"no information" in updateModule():

switch(g_model.moduleData[module].type) {
  ...
  case MODULE_TYPE_FLYSKY: 
          name->setText("FLYSKY");
          break;
  ...
  case MODULE_TYPE_NONE:
          name->setText("OFF");
          break;
  ...
  default:
         name->setText("shouldn't happen");
}
gagarinlg commented 1 year ago

This looks good!

mha1 commented 1 year ago

got rid of "no information" module names. Module names are now identical to the settings internal and external RF dialog, expect PXX2 which sets name depending on hw info.

image

pfeerick commented 1 year ago

That looks a lot better! 😍

mha1 commented 1 year ago

@pfeerick anything else I can do?

pfeerick commented 1 year ago

Hm... how about we hide the status line when there is nothing to show? i.e. int_module_status_w and ext_module_status_w (wait, what... ext_module_name_w was used twice so there's no ext_module_status_w 😮 ) can go bye bye if MODULE_TYPE_NONE ("OFF" should be sufficient there! 😆 ) or a module type that doesn't give any status.

Other than that, it's perfect (well, nearly perfect... I have another change in mind, but need to add/check if another function exists before can do that... so a 2.9 problem). I'll give you a hint... we need a pretty print module helper func for module names ;)

mha1 commented 1 year ago

Actually that was the reason I put all the _w in the class definition because I thought of being able to hide/unhide lines like the PXX2 stuff. I'll try an update for the status lines. And thanks for pointing out my most favorite oops. The famous copy with incomplete edit after paste error

mha1 commented 1 year ago

image image image

pfeerick commented 1 year ago

Perfecto! :)

mha1 commented 1 year ago

I added a commit to https://github.com/EdgeTX/edgetx/pull/2325. I hope without my favorite error.