IRC Tramp CLI Power Tables

[SomeTestStuffDoesntWork]

Summary

• Added new configuration array to support IRC tramp configurations
  • Consists of five integers
  • Defaults to -1 on reset
  • Set by CLI via: tramp_pl_override <pl_idx> <pl_val_milliwatts> or tramp_pl_table <pl1_mw> <pl2_mw> ... <pl_n_mw>
    • tramp_pl_override can be used to override the default tramp tables without losing backwards compatability. For example, if a user defines PL 1 but no others, then only PL 1 is changed and the defaults for the other power levels are used. Alternatively, if the default table contains three power level entries but the user overrides up to entry 4, then the additional entry is added and the table in use can use up to PL 4. If <pl_val_milliwatts> is set to -1, the power level entry is ignored and the default for that entry is used. Again, this is a bit of a strange solution but it was done to allow for full PL configurability while retaining all prior tramp PL behavior when the new configurations are left unmodified.
    • tramp_pl_table can be used to define an entirely new table in one command to take the place of the defaults. If less than the max number of power levels are sent, then the remaining power levels in the table are filled with that of the last power level sent (IE tramp_pl_table 25 100 200 will fill PLs 3, 4, and 5 with 200 mw)
  • Can be used for all VTX powers up to what can be stored in a uin16_t
• Added tramp config dump command to view current tramp config/statistics
  • Can be run by issuing tramp_dump_config
  • Dumps:
    • Current number of valid entries in the power level table
    • Current power levels in use
    • Frequency as reported by VTX
    • Power as reported by VTX
    • Power requested by FC
    • Frequency requested by FC
    • Power index reported by FC
  • Allows users to determine if the VTX accepted the PL/frequency values expected and aids in diagnosing issues with stubborn transmitters

Testing

This has been tested with an ATOMRC F405 NAVI on the ground communicating with a stubborn VTX. This VTX was particulary impacted by the default tables as its PLs are set as 1, 2, and 3 in place of milliwatt values, making the defaults impossible to use. Changing PL entries 1, 2, and 3 to 1, 2, and 3 milliwatts respectively fixed this issue and the VTX correctly transitioned to higher PLs on command. Non-volatile storage was tested and saved values were recovered across power cycles. Configuration reset was also tested and all values initialized to -1 as expected. Additional board tests may be required.
Please Note: This contains a settings change and will most likely require a diff all + full erase + reapplication of original config in order to perform further testing.

Related

This may fix the following:

• IRC Tramp RF Power Selection Enhancement #8340 (closed, but did not have a long-term solution. Contains the most comprehensive description of the issue)

Other Notes

I apologize in advance for any coding standards inconsistencies. I regularly use a slightly different set and some issues may have slipped through. Additionally, I have left a fair quantity of comments. Some of these may not be as useful, but I tend to err on the side of more comments rather than less to assist with later maintenance. I can remove the majority of these if they just add noise.

[github-actions]

Branch Targeting Suggestion

You've targeted the master branch with this PR. Please consider if a version branch might be more appropriate:

• maintenance-9.x - If your change is backward-compatible and won't create compatibility issues between INAV firmware and Configurator 9.x versions. This will allow your PR to be included in the next 9.x release.

• maintenance-10.x - If your change introduces compatibility requirements between firmware and configurator that would break 9.x compatibility. This is for PRs which will be included in INAV 10.x

If master is the correct target for this change, no action is needed.

---

This is an automated suggestion to help route contributions to the appropriate branch.

[sensei-hacker]

Thanks for doing your first INAV PR! A lot of people will be very happy about this, because they've been asking for it for a long time. This is great, if it works out.

You mentioned comments. Let me comment on comments. :)
We have this code:

if(isEmpty(cmdline))
{
    cliPrint(parseErrString);
    return;
}

That's great code - totally self-explanatory. Someone who isn't even a programmer could likely read and pretty much understand that because it reads pretty much like English prose. If cmdLine is empty, print an error and return. Awesome.

THEREFORE, the comment repeating what the code says may be unnecessary noise that actually ends up making the code harder to read by making it longer:

if(isEmpty(cmdline))
{
    // Nothing sent, so fail and bail.
    cliPrint(parseErrString);
    return;
}

There's also the question there of -- are the comments before the code or after? Hmm, both?
Typically if a comment was needed, we'd put it before the code is is explaining:

    // Nothing sent, so fail and bail.
    if(isEmpty(cmdline))
    {
        cliPrint(parseErrString);
        return;
    }

But here there is nothing to explain - it's great, self-evident code.

Typically, comments should describe WHY the code is doing something that isn't obvious.
It would answer a question the reader might have - "why did they do that instead of just doing ...?"
This doesn't gain from a comment:

      sum = sum + newValue;

This certainly might!

        sum = sum + 0.0;

A comment could say 0.0 is added to make it a float. ( Though maybe explicitly casting to a float would be better in that example.)

We have a function called constructPowerTable(). Which contructs the power table, of course.
The comment "// Construct the power table" is just a distraction because the function is literally called constructPowerTable.

[SomeTestStuffDoesntWork]

Thanks for doing your first INAV PR! A lot of people will be very happy about this, because they've been asking for it for a long time. This is great, if it works out.

You mentioned comments. Let me comment on comments. :) We have this code:

if(isEmpty(cmdline))
{
    cliPrint(parseErrString);
    return;
}

That's great code - totally self-explanatory. Someone who isn't even a programmer could likely read and pretty much understand that because it reads pretty much like English prose. If cmdLine is empty, print an error and return. Awesome.

THEREFORE, the comment repeating what the code says may be unnecessary noise that actually ends up making the code harder to read by making it longer:

if(isEmpty(cmdline))
{
    // Nothing sent, so fail and bail.
    cliPrint(parseErrString);
    return;
}

There's also the question there of -- are the comments before the code or after? Hmm, both? Typically if a comment was needed, we'd put it before the code is is explaining:

    // Nothing sent, so fail and bail.
    if(isEmpty(cmdline))
    {
        cliPrint(parseErrString);
        return;
    }

But here there is nothing to explain - it's great, self-evident code.

Typically, comments should describe WHY the code is doing something that isn't obvious. It would answer a question the reader might have - "why did they do that instead of just doing ...?" This doesn't gain from a comment:

      sum = sum + newValue;

This certainly might!

        sum = sum + 0.0;

A comment could say 0.0 is added to make it a float. ( Though maybe explicitly casting to a float would be better in that example.)

We have a function called constructPowerTable(). Which contructs the power table, of course. The comment "// Construct the power table" is just a distraction because the function is literally called constructPowerTable.

This all sounds good to me. Took a pass through and cleaned up the comments.

Additionally, this commit should fix the unused static function errros from SITL builds. Had the commands in preprocessor conditionals but not the static functions used to service those commands. The functions should now follow the command definitions

[robotgoat]

Hi @SomeTestStuffDoesntWork this is a great addition and something I've been looking for in INAV since I have some IRC Tramp based VTXs that won't work properly in the current implementation. I'm in the process of testing your code applied to INAV 9.0.1 and came across some problems as I wasn't fully able to replicate the success you had in your PR.

I was able to fix the first problem (though not sure if it is correct), which is that default and min values for the new cli variable vtx_pw_override_mw fails the system checks in INAV configurator. To fix this, in settings.yaml, I changed the type to unit16_t, default_value to 0, and min to 0. Additionally, when declaring the vtxTrampOverride struct in vtx_tramp.h, the struct's name seems to be missing so I changed it to:

typedef struct vtxTrampPwOverride {
    int vtxPwOverrideMw;
} vtxTrampPwOverride_t;

I've confirmed with these two changes that the firmware builds correctly and does not throw any system errors in INAV Configurator.

For reference, the FC that I'm testing this on is the AtomRC F405 Navi and the Jhemcu Ruibet Tran 3016 VTX.

After doing a full erase flash with the modified firmware, I applied the VTX power table using tramp_pl_table because I don't care about the default table. I use the cli command: tramp_pl_table 25 200 400 800 1600 and the value sets as expected. I then set the power value in the Configuration tab like I normally would for any other VTX to test that all the power levels work. For some strange reason, I'm able to confirm that power levels 1, 2, and 3 that pertain to 25mw, 200mw, and 400mw work as expected (confirmed through the red LED flashes on the VTX). However, it doesn't work for power level 4 (800mw) or 5 (1600mw). When I query what the config is doing with tramp_config_dump, it appears that for power level 4, the FC is sending 600mw to the VTX.

I hardcoded the table the old way as described in this discussion #8340 and was able to confirm all 5 power values work as expected once I force INAV to use the hardcoded table for the 1600mw VTX using the set vtx_max_power_override = 1600 command.

Would you happen to know why this could be?

Additionally, I don't fully understand the purpose of the variable vtxPwOverrideMw that you create. It seems that once I define my custom table, the value of vtxPwOverrideMw remains at 25. Also, is there a conflict when both vtxPwOverrideMw and vtx_max_power_override are used? Is one supposed to superscede the other?

I think your implementation could also be refined if there is no default table at all. To simplify the code and usability, would it make sense to have the user manually define the power values for their specific IRC Tramp vtx that way there's little to no confusions about what's overriding what?

Thanks for your help with helping me understand your code better!

[github-actions]

Test firmware build ready — commit 831a023

Download firmware for PR #11217

228 targets built. Find your board's .hex file by name on that page (e.g. MATEKF405SE.hex). Files are individually downloadable — no GitHub login required.

Development build for testing only. Use Full Chip Erase when flashing.

[SomeTestStuffDoesntWork]

Hi @robotgoat, thanks for taking a look at this PR! I apologize for any issues that have made their way into the latest commit and I am working to resolve them locally. I should have updates ready to try out again within the next few days.

The power overrides (as they currently function) consist of five individual table entries that each can contain a single power level. When the power levels are selected during VTX initialization, either the hard coded values are chosen or if the associated power override table entry contains a custom user value, the power override is used. Since the table construction is currently dictated by the size of the default table selected (IE if the default for the power reported by the VTX contains four entries, only four entries can be overridden), it may be possible that some of the other assigned power levels are getting truncated. I will try to resolve this issue as it does pose a limitation unless the vtx_max_power_override is used to force selection of a power group with all five entries (vtx_max_power_override can still be used as the override swap takes place after the vtx_max_power_override's operations are applied). While this is a bit of redirection it was the best compromise I could come up with to make sure that all prior VTX operations still work with existing configurations while also allowing custom tables to slip in underneath as an additional layer. This could be simplified with the removal of the defaults, but that would force the use of the power table and also probably require a configurator change, all of which I am a bit hesitant to suggest unless there is significant interest in that approach.

I'll also take a look at removing the individual power level override commands in favor of the the tramp_pl_table command as it introduces a bit of noise that doesn't buy much additional function. With that removed, some of the individual default-value checking can go away in favor of a single check on the first value.

[robotgoat]

@SomeTestStuffDoesntWork no problem! Thank you for taking time to make this feature available and explaining how it works.

I agree with your implemention to keep the default list of power tables while adding the custom power table override feature as an option as it's the least destructive while giving users this additional feature. I also think it will be a good idea to remove the individual power level override.

Thank you for explaining how the existing cli variable vtx_max_power_override plays into this. From what I understand, I think it makes sense to trust the user to input the correct power table values with your tramp_pl_table command. That table will then be the active one unless overridden by vtx_max_power_override.

One additional thing that your implementation adds is vtxPwOverrideMw. After applying a custom table, that variable shows up in a diff all command and I don't understand what it's doing since it never changes its value. I'm not sure if I missed it in your explaination, but I didn't see any mention about this variable.