New naming convention of variables linked to a parameter

[bresch]

Description

After a discussion with @MaEtUgR , @bkueng and @Stifael we came to the conclusion that all the variables that are directly linked to a parameter (declared using
(ParamType<px4::params::PARAMETER_NAME>) variable)
should have the same naming convention to be easy to grep for.

The following convention was used in the flight tasks:
(ParamType<px4::params::PARAMETER_NAME>) PARAMETER_NAME
The problem is that ALLCAPS is usually only used for #defines in C/C++ and should not used to name a variable.

We then decided that the following naming makes more sense:
(ParamType<px4::params::PARAMETER_NAME>) _param_parameter_name
The reasons are:

• snake_case is used to declare variable, this is a general convention in PX4
• grepping is easy since the variable contains the parameter name
• autocompletion is easy since all the parameters start with _param

After trying with sed and perl in a bash script, I finally decided to use python since it's easier to handle the corner cases.

Status:

I'm converting the modules one by one to keep the commits small.

• drivers
• templates
• lib
• modules
  • commander
  • events
  • ekf2
  • fw_pos_control
  • load_mon
  • local_pos_estimator
  • simulator
  • wind_estimator
  • mavlink
  • mc_att_control
  • mc_pos_control
  • navigator
    - [ ] pass/fail script for CI Will be in a separate PR

@dagar for the CI script, can it be in Python?

Replaces #11582

FYI: @BazookaJoe1900 @mcsauder @mhkabir

[bresch]

Here is the current script:

#!/usr/bin/env python
# -*- coding: utf-8 -*-

import re
import fileinput
import sys

filenames = []
for filename in sys.stdin:
    filenames.append(filename.rstrip())

pattern = r'^.*<px4::params::([A-Za-z0-9_]+)>\)\s*([A-Za-z0-9_]+)[,\s]?.*$'
regex = re.compile(pattern, re.MULTILINE)

replace_dict = dict()

print("Searching for parameters...")
for filename in filenames:
    print("In {} :".format(filename))
    with open(filename, 'r') as f:
        text = f.read()
        for match in regex.finditer(text):
            #print("Line: " + match.group(0) + '\t' + match.group(1) + " and " + match.group(2).lower())
            replace_dict[match.group(2)] = "_param_" + match.group(1).strip().lower()

print("The following variables will be changed")
for old_var in replace_dict:
    print("{} -> {}".format(old_var, replace_dict[old_var]))

for filename in filenames:
    print("In {} :".format(filename))
    for old_var in replace_dict:
        for line in fileinput.input(filename, inplace=1):
            line = re.sub(r'(>\)\s*)\b' + old_var + r'\b', r'\1' + replace_dict[old_var], line.rstrip()) # replace the declaration
            line = re.sub(r'(^\s*)\b' + old_var + r'\b', r'\1' + replace_dict[old_var], line.rstrip()) # replace the 2 liner declaration
            line = re.sub(r'\b' + old_var + r'\b(\.[gs]et\()',replace_dict[old_var] + r'\1' , line.rstrip()) # replace the usages (with get/set)
            print(line)

usage rg -l '\.[gs]et\(|px4::params::' | python parameter_update.py where rg is RIPgrep

[hamishwillee]

@bresch Recommend that you update the docs after this merges. Possibly in https://dev.px4.io/en/advanced/parameters_and_configurations.html#parameter-names , but definitely with a note near the code example in https://dev.px4.io/en/advanced/parameters_and_configurations.html#c-api

[MaEtUgR]

Cool! Forcing consistency is even better. Convention also sounds good. The only downside is no direct copy-paste because it needs to be lowercased and prepended but that's a good compromise.

grepping is easy since the variable contains the parameter name

is in my eyes the most important advantage and will improve efficiency when scanning code.

[CarlOlsson]

This is really nice. Would it also be possible to have a naming convention for when we use topics? I.e. vehicle_local_position is sometimes called local_pos
https://github.com/PX4/Firmware/blob/14ef009aabf5723b1a9f8fe5489844c961f05fab/src/modules/commander/Commander.cpp#L875-L875
and sometimes lpos
https://github.com/PX4/Firmware/blob/14ef009aabf5723b1a9f8fe5489844c961f05fab/src/modules/commander/Commander.cpp#L1111-L1111

[MaEtUgR]

@CarlOlsson Good idea, I consistently switched to _sub_[topic name]/_pub_[topic name] for the handle or uORB::Subscription/uORB::Publication wherever I changed anything e.g.:
https://github.com/PX4/Firmware/blob/8cdc2d9ae91417944a6698d821b3789cf8147715/src/lib/FlightTasks/tasks/FlightTask/FlightTask.cpp#L22 But let's do one step at a time and not everything at once.

[bresch]

@dagar Now that I modified all the parameters, if I run the script from Firmware/src, I still have 3 weird changes:

---------- src/lib/FlightTasks/tasks/AutoLine/FlightTaskAutoLine.cpp ----------
index 31c6016..8dab82f 100644
@@ -267,4 +267,4 @@ void FlightTaskAutoLine::_generateAltitudeSetpoints()
 		_velocity_setpoint(2) = 0.0f;
 		_position_setpoint(2) = _target(2);
 	}
-}
\ No newline at end of file
+}

---------- src/lib/FlightTasks/tasks/Failsafe/FlightTaskFailsafe.cpp ----------
index db3604e..9a84ab2 100644
@@ -74,4 +74,4 @@ bool FlightTaskFailsafe::update()
 
 	return true;
 
-}
\ No newline at end of file
+}

------------------ src/lib/parameters/px4params/srcparser.py ------------------
index 5098545..3d22fac 100644
@@ -320,7 +320,7 @@ class SourceParser(object):
                     self.param_groups[group].AddParameter(param)
                 state = None
         return True
-    
+
     def IsNumber(self, numberString):
         try:
             float(numberString)

[dagar]

@bresch you're losing the last newline. Without digging into the specifics of your script you could try finishing by inserting a final newline. For the srcparser.py I would simply structure it such that it only runs on .cpp and .h.hpp files.

[bresch]

@dagar Can we merge that now?

[MaEtUgR]

Now that I modified all the parameters, if I run the script from Firmware/src, I still have 3 weird changes:

The first two it finishes the files with a newline and the third one it removes trailing whitespaces. Even though I'm not sure if the changes are indtended they look correct to me.

Not specific to this PR but isn't PX4 consistently indented with tabs? The scripts in the Tools folder have inconsistent indentations...

[MaEtUgR]

All the parameters I manually scanned looked like expected. I also did some simulation tests with switching parameters just to be sure and it was all fine. It's all random testing and not extensive.

[bresch]

@MaEtUgR Thanks for the review! I converted the spaces into tabs in the python script and rebased on master

[julianoes]

Nono, please Python with spaces! I strongly vote that we use PEP8 for Python scripts which recommends spaces: https://www.python.org/dev/peps/pep-0008/#tabs-or-spaces

[bresch]

@julianoes Ok, I removed the last commit :D

[MaEtUgR]

It's the same again and I already reviewed it.