Configurable serial ports v2 #10337

bkueng · 2018-08-27T10:20:50Z

Rebased and reworked version of #9615. Differences:

per-module serial config parameters instead of per-port as asked for by Daniel
- added the required port deconfliction in the startup scripts (eventually the user should be notified about the conflict in some way, or prevent a conflicting config in the first place).
Added module config files based on YAML

per-module vs. per-port config params

Personally I don't see strong arguments for either way, both have their drawbacks:

per-port (previous PR)

more centralized approach (in terms of how modules register a serial command)
QGC metadata contains all modules even if not enabled

per-module (this PR)

explicit port deconfliction required
More parameters (one per service instead of one per port)
QGC metadata contains all ports

In any case we have now both versions.

Module Config Files

Modules now need a config file to register a startup command, and configure the parameters. I decided to go for YAML, because:

generally wide-spread tooling support
benefits over JSON:
- simpler to read and edit
- YAML is a superset of JSON
- we can have comments
- JSON is targeted more for serialization instead of configuration

Here's an example to register a serial command (see the mavlink config for an example of using multiple instances):

module_name: LeddarOne Rangefinder
serial_config:
    - command: leddar_one start -d ${SERIAL_DEV}
      port_config_param:
        name: SENS_LEDDAR1_CFG
        group: Sensors

These config files can also be used for parameter definitions. MAVLink already uses them to generate multiple instances. These are then translated into the existing C-style parameter definitions. This allows us to transition all files step by step and eventually remove the translation layer.

In addition it allows us to do schema validation, to ensure that the configs will conform to the expectations. I added a script that uses cerberus (see commit 1b42c63). The schema is also a good place to document all possible options.

Once there's consensus, I'll continue here, there's still quite a bit left to do.

dagar · 2018-08-27T13:26:14Z

Looking good from what I've seen so far. I'll give it a try later.

To be honest I still think it would be easier and more scalable to have modules directly read their own configuration (parameters per instance), but it looks like you've found a good compromise.

dagar · 2018-08-27T13:30:29Z

cmake/configs/nuttx_px4fmu-v2_default.cmake

+	TEL1:/dev/ttyS1
+	TEL2:/dev/ttyS2)
+
+set(px4_constrained_flash_build 1)


We might want to look at cmake options for this kind of thing.
https://cmake.org/cmake/help/v3.0/command/option.html

dagar · 2018-08-27T13:34:12Z

CMakeLists.txt

@@ -473,8 +475,12 @@ add_custom_target(metadata_airframes
 	USES_TERMINAL
 )

+file(GLOB_RECURSE yaml_config_files ${PX4_SOURCE_DIR}/src/modules/*.yaml


It's worth getting rid of all cmake globbing to avoid possible surprises with incremental builds.

Absolutely. This was just the quickest way to achieve it. Essentially I want to tell cmake 'give me all module configs for all possible modules'. And adding a new config with all modules does not seem right for that.

dagar · 2018-08-27T13:54:07Z

Just curious, but did you consider toml (vs json vs yaml)?

LorenzMeier · 2018-08-27T13:56:41Z

Very cool! I need to quickly go with you through it, but it looks very scalable and per-airframe accurate now.

dagar · 2018-08-27T14:25:52Z

Tools/serial/generate_config.py

+        "index": 6,
+        "default_baudrate": 57600,
+        },
+    "URT3": { # for Omnibus


I think we'll end up needing the full range of generic names (especially for the linux boards).

bkueng · 2018-08-27T14:30:47Z

Just curious, but did you consider toml (vs json vs yaml)?

Yes briefly. It's a valid alternative, but it's just not as wide-spread used (yet), also in terms of tooling. And nested structures seem to be better supported in YAML.

dagar · 2018-08-27T14:38:22Z

src/lib/parameters/CMakeLists.txt

 		--xml ${parameters_xml}
 		--inject-xml ${CMAKE_CURRENT_SOURCE_DIR}/parameters_injected.xml
 		--overrides ${PARAM_DEFAULT_OVERRIDES}
 		--board ${BOARD}
 		#--verbose
 	DEPENDS
 		${param_src_files}
+		${generated_params_dir}/serial_params.c


Use the same variable as the custom command OUTPUT - ${generated_serial_params_file}

dagar · 2018-08-27T14:54:30Z

Tools/serial/rc.serial.jinja

+
+# make sure all baudrates are marked as used
+param touch
+{%- for serial_device in serial_devices %} SER_{{ serial_device.tag }}_BAUD


This could be skipped so that it's only visible to the user when on an appropriate config.
For example frsky telem, iridum, syslink, etc shouldn't present a configurable baudrate.

This is a per-port setting, e.g. SER_TEL1_BAUD. It will make it a bit easier to use.

Yes and they'll still be there if you have anything (Mavlink, GPS, etc) enabled on that port. In the cases where it's not used we can hide it, skip the sync, and close one more little hole where users are able to try doing something that doesn't make any sense. Since all of these parameters set via command line are reboot_required I don't really see the problem (set mavlink, reboot, serial mavlink baudrate param available).

This is a case where handling the param in the module itself could actually be a lot better as it would enable runtime changes.

…thon packages Required for PX4/PX4-Autopilot#10337

bkueng · 2018-09-05T15:33:35Z

This is now in a working state, ready for review and CI tests.

Testing

I tested these boards:

Pixhawk (v2) (the SERIAL 4 port can now be used as well)
Pixhawk 3 Pro (v4pro)
Pixracer (v4)
Pixhawk 4 (v5)
Intel Aero (aerofc-v1)
Omnibus-f4sd

Compatibility

SYS_COMPANION is deprecated and can be removed after the next release. I added code that automatically maps existing SYS_COMPANION settings to the new parameters. So users who have SYS_COMPANION set should not notice a difference (though I did not add the translation for less common configs like frsky telemetry).

Breaking Changes

Telem 2 is disabled by default to reduce RAM usage
SENS_EN* parameters for serial distance sensors got replaced with the generic serial config params, and a user needs to update.

Depends on PX4/PX4-containers#133

hamishwillee · 2018-09-05T23:04:11Z

@bkueng There will probably result in quite a lot of docs updates. I'm thinking about

everywhere we suggest setting SYS_COMPANION
how to enable various sensors etc.
How others can create config params and where. Probably best for you to write the docs on this one and me to review - I assume based on your info above.

Lets talk them through when this gets accepted.

Is TELEM2 disabled by default on everything or just FMUv2?

…thon packages (#133) Required for PX4/PX4-Autopilot#10337

dagar · 2018-09-06T06:48:13Z

ROMFS/px4fmu_common/init.d/3100_tbs_caipirinha

+	param set MAV_1_CONFIG 2
+	param set MAV_1_RATE 1000
+	param set MAV_1_MODE 3
+	param set SER_TEL2_BAUD 57600


In my opinion this should be dropped. There's nothing about a TBS caipirinha that requires a specific telemetry 2 configuration. The airframe can easily be kept generic.

Agreed, dropped.

dagar · 2018-09-06T06:56:50Z

Tools/serial/generate_config.py

+        "index": 6,
+        "default_baudrate": 0,
+        },
+    "URT6": { # for Omnibus


There are boards that just have a range of generic uarts. Should we add them here now?

I'd add them as needed.

It includes boards we already have in tree (navio2, av-x, nucleos, etc). If we add the full range initially we can keep the index simple and consistent.

Ok feel free to add them and push to the branch.

…thon packages (#133) Required for PX4/PX4-Autopilot#10337

bkueng · 2018-09-06T09:41:09Z

@hamishwillee yes all of these need to be updated.
TELEM2 is disabled by default on all boards. This is a port that needs to be configured according to a users use-case, so I think leaving it disabled makes sense. But if others feel different I'd like to hear it.

hamishwillee · 2018-09-06T22:59:27Z

TELEM2 is disabled by default on all boards. This is a port that needs to be configured according to a users use-case, so I think leaving it disabled makes sense. But if others feel different I'd like to hear it.

Completely agree - part of the problem with SYS_COMPANION was that it always had to be disabled or set in order to use the port. What I don't understand yet is how the user would enable it. I think from the discussion this would be done using per-module (?) parameter(s) appropriate to that port. I guess that is what your docs will answer :-)

bkueng · 2018-09-07T06:49:12Z

I think from the discussion this would be done using per-module (?) parameter(s) appropriate to that port. I guess that is what your docs will answer :-)

Yes there's a per-module param. For example for FrSky telemetry there's a parameter TEL_FRSKY_CONFIG that provides a drop-down list with the Ports where you would select Telem2.

Replaced with the more general serial config params.

…r-port

Replaced with the more generic serial config params. rc.mavlink contains automatic transition support that can be removed after the next release.

Can be reverted if needed later on...

This is because NuttX uses a different assignment for variables.

dagar · 2018-09-23T14:12:32Z

src/modules/mavlink/mavlink_main.cpp

@@ -681,6 +682,11 @@ int Mavlink::mavlink_open_uart(int baud, const char *uart_name, bool force_flow_
 	case 1500000: speed = B1500000; break;
 #endif

+#ifdef B2000000


Where is this missing?

I think not all Linux/Posix systems have it.

dagar · 2018-09-23T14:17:21Z

Looks good to me.

Let's regroup after this settles in master and note what's still needed.

How to handle QGC side configuration
missing board configurations and posix handling
px4 side changes to prevent conflicts and provide actionable error messages
path forward for module yaml files and parameters
- let's not have 2 levels of code generation long term
QGC metadata update ([WIP] Configurable serial ports #9615 (comment))

bkueng · 2018-09-24T06:14:23Z

Thanks for the review @dagar.

@PX4/testflights can you test this? You don't need to fly, just make sure all UARTs are working as expected on different boards: telemetry link, gps, serial sensors etc.

Avysuarez · 2018-09-24T23:11:48Z

I tested on pixhawk 4 and pixhawk 2.1 the two FC works properly.

santiago3dr · 2018-09-24T23:42:29Z

looks to be working on the pixracer
...only port that doesn't is telem2 which was disabled with this pr so no surprises

bkueng · 2018-09-25T05:53:25Z

Thanks @Avysuarez and @santiago3dr. Let's merge.

DonLakeFlyer · 2018-09-25T16:29:51Z

Anything QGC needs to do here?

dagar · 2018-09-25T16:54:50Z

@DonLakeFlyer short term getting the metadata to auto update would be a helpful improvement. #9615 (comment)

It doesn't need to happen immediately, but I'd also like to get a QGC configure screen for the serial ports (per board metadata) that pulls together all the possible options for each port. We should probably discuss this on a call.

DonLakeFlyer · 2018-09-25T17:35:08Z

@dagar Can you start QGC issue for the serial port page and a separate one for parameter meta data auto-update?

dagar · 2018-10-01T13:33:18Z

@dagar Can you start QGC issue for the serial port page and a separate one for parameter meta data auto-update?

Serial port configuration page - mavlink/qgroundcontrol#6905
Parameter metadata autoupdate - mavlink/qgroundcontrol#6906

dagar self-requested a review August 27, 2018 13:15

dagar added the Admin: Enhancement (improvement) 💡 label Aug 27, 2018

dagar reviewed Aug 27, 2018

View reviewed changes

dagar added this to the Release v2.0.0 milestone Aug 27, 2018

dagar reviewed Aug 27, 2018

View reviewed changes

bkueng added a commit to PX4/PX4-containers that referenced this pull request Sep 4, 2018

Dockerfile_base, archlinux_install_script: add pyyaml and cerberus py…

d4ab89a

…thon packages Required for PX4/PX4-Autopilot#10337

bkueng mentioned this pull request Sep 4, 2018

Dockerfile_base, archlinux_install_script: add pyyaml and cerberus python packages PX4/PX4-containers#133

Merged

bkueng force-pushed the configurable_serial_ports_v2 branch from 1b42c63 to b8a7080 Compare September 5, 2018 15:20

bkueng changed the title ~~[WIP] Configurable serial ports v2~~ Configurable serial ports v2 Sep 5, 2018

dagar pushed a commit to PX4/PX4-containers that referenced this pull request Sep 6, 2018

Dockerfile_base, archlinux_install_script: add pyyaml and cerberus py…

e2212fe

…thon packages (#133) Required for PX4/PX4-Autopilot#10337

dagar reviewed Sep 6, 2018

View reviewed changes

bkueng force-pushed the configurable_serial_ports_v2 branch from b8a7080 to a84fea5 Compare September 6, 2018 09:34

dagar pushed a commit to PX4/PX4-containers that referenced this pull request Sep 6, 2018

Dockerfile_base, archlinux_install_script: add pyyaml and cerberus py…

adf7046

…thon packages (#133) Required for PX4/PX4-Autopilot#10337

bkueng force-pushed the configurable_serial_ports_v2 branch from a84fea5 to 9467639 Compare September 12, 2018 07:35

bkueng mentioned this pull request Sep 12, 2018

about baudrate of serial #10450

Closed

dagar mentioned this pull request Sep 17, 2018

serial generate config add 10 generic serial ports #10492

Merged

bkueng force-pushed the configurable_serial_ports_v2 branch from bf88a39 to 1d1d5ee Compare September 20, 2018 06:13

bkueng and others added 8 commits September 22, 2018 09:06

params: remove unused SENS_EN_* params

15d993a

Replaced with the more general serial config params.

Tools/serial: use per-module serial port config params, instead of pe…

aadd938

…r-port

serial drivers/modules: add yaml config files

8be6a4c

SYS_COMPANION: deprecate this param

b0c0285

Replaced with the more generic serial config params. rc.mavlink contains automatic transition support that can be removed after the next release.

rc.serial.jinja: remove 'param touch' for baudrates

4657334

Can be reverted if needed later on...

shellcheck: ignore SC2154 on NuttX

b3d1af9

This is because NuttX uses a different assignment for variables.

serial generate config add 10 generic serial ports

3bb734e

Jenkins: add module config validation to CI

69b098b

bkueng force-pushed the configurable_serial_ports_v2 branch from 92b459c to 69b098b Compare September 22, 2018 07:08

dagar reviewed Sep 23, 2018

View reviewed changes

dagar approved these changes Sep 23, 2018

View reviewed changes

bkueng mentioned this pull request Sep 24, 2018

Trying to start WIFI connection on TELEM1 does not work #10549

Closed

dagar requested a review from a team September 24, 2018 14:16

bkueng merged commit 525531f into master Sep 25, 2018

bkueng deleted the configurable_serial_ports_v2 branch September 25, 2018 05:53

bkueng mentioned this pull request Sep 25, 2018

rcS: start frsky_telemetry on Pixracer if not enabled already #10562

Merged

bkueng mentioned this pull request Sep 26, 2018

add a page to document the serial port configuration PX4/PX4-user_guide#352

Merged

This was referenced Oct 1, 2018

Configurable serial ports mavlink/qgroundcontrol#6905

Open

Firmware metadata autoupdate mavlink/qgroundcontrol#6906

Open

MaEtUgR mentioned this pull request Nov 15, 2022

Problems with the full configuration parameters #20593

Open

junwoo091400 added the Architecture label Dec 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Configurable serial ports v2 #10337

Configurable serial ports v2 #10337

bkueng commented Aug 27, 2018 •

edited by AuterionWrikeBot

Loading

dagar commented Aug 27, 2018 •

edited

Loading

dagar Aug 27, 2018

dagar Aug 27, 2018

bkueng Aug 27, 2018

dagar commented Aug 27, 2018

LorenzMeier commented Aug 27, 2018

dagar Aug 27, 2018

bkueng commented Aug 27, 2018

dagar Aug 27, 2018

dagar Aug 27, 2018

bkueng Aug 27, 2018

dagar Aug 27, 2018

bkueng commented Sep 5, 2018

hamishwillee commented Sep 5, 2018

dagar Sep 6, 2018

bkueng Sep 6, 2018

dagar Sep 6, 2018

bkueng Sep 6, 2018

dagar Sep 6, 2018

bkueng Sep 7, 2018

bkueng commented Sep 6, 2018

hamishwillee commented Sep 6, 2018

bkueng commented Sep 7, 2018

dagar Sep 23, 2018

bkueng Sep 24, 2018

dagar commented Sep 23, 2018 •

edited

Loading

bkueng commented Sep 24, 2018

Avysuarez commented Sep 24, 2018 •

edited

Loading

santiago3dr commented Sep 24, 2018

bkueng commented Sep 25, 2018

DonLakeFlyer commented Sep 25, 2018

dagar commented Sep 25, 2018

DonLakeFlyer commented Sep 25, 2018

dagar commented Oct 1, 2018

Configurable serial ports v2 #10337

Configurable serial ports v2 #10337

Conversation

bkueng commented Aug 27, 2018 • edited by AuterionWrikeBot Loading

per-module vs. per-port config params

per-port (previous PR)

per-module (this PR)

Module Config Files

dagar commented Aug 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dagar commented Aug 27, 2018

LorenzMeier commented Aug 27, 2018

Choose a reason for hiding this comment

bkueng commented Aug 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkueng commented Sep 5, 2018

Testing

Compatibility

Breaking Changes

hamishwillee commented Sep 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkueng commented Sep 6, 2018

hamishwillee commented Sep 6, 2018

bkueng commented Sep 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dagar commented Sep 23, 2018 • edited Loading

bkueng commented Sep 24, 2018

Avysuarez commented Sep 24, 2018 • edited Loading

santiago3dr commented Sep 24, 2018

bkueng commented Sep 25, 2018

DonLakeFlyer commented Sep 25, 2018

dagar commented Sep 25, 2018

DonLakeFlyer commented Sep 25, 2018

dagar commented Oct 1, 2018

bkueng commented Aug 27, 2018 •

edited by AuterionWrikeBot

Loading

dagar commented Aug 27, 2018 •

edited

Loading

dagar commented Sep 23, 2018 •

edited

Loading

Avysuarez commented Sep 24, 2018 •

edited

Loading