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

generalize serial port configuration #8302

Closed
dagar opened this issue Nov 16, 2017 · 15 comments
Closed

generalize serial port configuration #8302

dagar opened this issue Nov 16, 2017 · 15 comments

Comments

@dagar
Copy link
Member

dagar commented Nov 16, 2017

We need to generalize serial port configuration (exposed as parameters) with appropriate defaults set per board.

For the most part these are hard coded, but with SYS_COMPANION (telem2) configurable. I'd like to extend that to all serial ports.

  • telem port x N
  • one or more GPS ports
  • generic (eg pixhawk serial 4/5)

Previous issue #5184

TODO

  • each serial needs a configuration parameter, and likely a baudrate parameter.
    • SERIAL_X, SERIAL_X_BAUD
  • each board needs to specify the available serial ports and set appropriate defaults
    • ie telem 1 = mavlink normal at 57600, etc
  • PX4 modules (drivers) need to register themselves in this configuration list
@dagar
Copy link
Member Author

dagar commented Feb 5, 2018

#8826

@dagar
Copy link
Member Author

dagar commented Feb 16, 2018

Thinking about this some more, I'm now considering placing the burden of configuration on each Driver/Module itself.

I don't see how we could handle configuration per serial port without maintaining large lists of every possible option. This starts to fall apart when you think about boards that only include a subset of Drivers, custom configurations, deleting old drivers, or vendor proprietary extensions.

The downside is PX4 itself would need to deconflict serial port usage. I think this is relatively easy to do if we use the Driver base class for serial devices as well.

@dagar
Copy link
Member Author

dagar commented Feb 16, 2018

For the board manifest we need to know.

  • which serial ports are available to the system
  • if some need to be hard coded (eg px4io)
  • default functionality that isn't hard coded (eg mavlink on telem2)

@dagar
Copy link
Member Author

dagar commented Feb 16, 2018

GPS

  • GPS1 serial port (per board default)
  • GPS2 serial port (per board default)

Mavlink

Serial port drivers standard configuration

  • port
  • baud

misc

  • frsky
  • crazyflie
  • iridium

@bkueng
Copy link
Member

bkueng commented May 14, 2018

How about this:
We add a script to generate params.c for N uarts (configured driver, baud, mavlink mode, ... params) and the necessary romfs code to start the corresponding drivers:

  • UART mapping: the script contains a per-board mapping from TELEM1, GPS1, ... to /dev/ttySx, and the params apply to the former: e.g. UART_TELEM1_BAUD, UART_GPS1_BAUD. The per-board config can also be pulled out into separate config files or the cmake config.
  • a user can easily specify per UART what to run, and the mapping makes it easy to know which UART it is (it matches the label on the board).
  • no conflicts: you specify what to run on a port, so you cannot run multiple things on a single port
  • hardcoded UARTs such as px4io are not included
  • the script contains a list of drivers that includes the required romfs startup command.
  • adding a new device is simple: just add a list entry to the script (device name & startup command) (or we can create a separate config file for that).
  • no/minimal code changes needed
  • generated romfs code can be kept small by using a while loop
  • To pass the params to the driver via CLI, we could use: set BAUD=`param show UART_X_BAUD` . But unfortunately this requires a tmpfs which we don't have. Alternatively we can do it as the pwm command does: -b p:<PARAM_NAME>, thus requiring only minimal code changes.

potential downsides:

  • QGC metadata is static (more or less), thus all existing drivers will be in the dropdown list. Though the script could easily handle this by excluding certain entries depending on the board. But the QGC default param XML must include all possible configs and UARTs. In both cases the user will only see the params for UARTs that the board actually has.
  • adding a new device requires editing a file at a specific location. It will require good documentation, then it's not an issue.

@dagar
Copy link
Member Author

dagar commented May 15, 2018

@bkueng that's what I wanted to do initially, but I see various issues with either maintaining lists of all possible serial drivers (+ range of configurations) or generating that list (parameter stability). Splitting baudrate helps, but there's more than that. You can end up with a large number of options that aren't possible or relevant for the specific build you're using. The other problem is that our init scripts are currently a bad way to handle this as they aren't even used on many platforms (or many custom nuttx configurations for that matter). We already have many frustrating configuration holes where active parameters shown to the user don't do anything.

What I was wondering about was pushing the configuration to each module as parameters plus special parameter types for specifying the port. Eventually we could have QGC collect all of these (imagine a board graphic where you click on each port) and let you select the configuration per port.

Pros

  • keeps everything modular and scalable across a wide range of configurations and setups (both upstream and custom)
  • users will only ever see relevant parameters for the specific setup they're connected to
  • works well in custom deployments that may add additional drivers and remove many of the existing options
  • avoid disconnect between command line arguments and parameters. Configuration via command is effectively inaccessible to most users. Init script level configuration has proven to be brittle for us.

Cons

  • requires more effort
  • getting to a centralized per UART configuration view in QGC requires much more work than a per UART param
  • likely need (or would prefer) a concept of parameter instances (eg configure mavlink 0, 1, 2, etc)

@hamishwillee
Copy link
Contributor

hamishwillee commented May 15, 2018

I'm sure you've through all this through ...

  1. For docs would be good if the solution allows us to (easily) generate mappings of board -> driver -> port mappings. Ie as previously discussed, be able to autogenerate list all sensors, and list the default port where they should be connected. Ultimately I would like to have docs in the drivers determine what appears in our peripherals docs.
  2. It should be very difficult to accidentally try run multiple things on the same port.
  3. Config should be dead easy

@bkueng
Copy link
Member

bkueng commented May 15, 2018

@dagar I agree, my proposal aims to be simple, user-friendly and implementable, not necessarily the best in the long-term (though it would allow for a QGC board graphic too). And I would prefer a solution where each module specifies its own configuration as well. But the current situation is already limiting for various users/use cases, among which:

  • dual GPS
  • frsky telemetry
  • RTPS
  • on which port to run a sensor driver
  • the omnibus board has only a limited amount of UARTs and thus a user needs the ability to decide what to run and where
  • newer boards such as the pixhawk pro and pixhawk 4 come with an additional UART port which is currently not configurable

And given that this issue has been long standing, we should aim for something that actually gets implemented sometime soon.

The other problem is that our init scripts are currently a bad way to handle this as they aren't even used on many platforms (or many custom nuttx configurations for that matter). We already have many frustrating configuration holes where active parameters shown to the user don't do anything.

I'm still for the posix shell, which would fundamentally solve this.

@hamishwillee agreed with all points. For the first one: you would be able to run every driver on any of the available UART ports, thus having maximum flexibility.

@dagar
Copy link
Member Author

dagar commented May 15, 2018

Sounds like we're on the same page. That would still definitely be an improvement if someone is able to implement it right now. Otherwise I'll eventually get around to it when I stumble across a solution that checks all the boxes.

Minor note - many of the distance sensor drivers evolved with their own configuration that could be rolled into this. A mix of boolean parameters to enable/disable, but with hard coded serial ports, or sometimes set in the init script.
https://github.com/PX4/Firmware/blob/master/src/drivers/distance_sensor/sf0x/parameters.c
https://github.com/PX4/Firmware/blob/master/ROMFS/px4fmu_common/init.d/rc.sensors#L427

I'm still for the posix shell, which would fundamentally solve this.

Yes!

@hamishwillee
Copy link
Contributor

Distance sensors should all definitely share the same approach (whatever it is), if only so that setup instructions can all be harmonised.

You both talk about this mysterious implementer - who is that likely to be?

Is there any docs on the posix shell solution? - just FMI (I'm interested on what this is, why this solves the problem, and why we don't use it). I fill like I'm missing an in-joke. If this is too much effort to point me to, don't worry about it.

@bkueng
Copy link
Member

bkueng commented May 16, 2018

You both talk about this mysterious implementer - who is that likely to be?

Haha, that is the biggest question :) Probably me or Daniel, whoever finds the time first.

Is there any docs on the posix shell solution? - just FMI (I'm interested on what this is, why this solves the problem, and why we don't use it). I fill like I'm missing an in-joke. If this is too much effort to point me to, don't worry about it.

It's a feature that so far has not been finished (#5162). Basically it would allow us to use the NuttX startup scripts also on Linux.

@dagar
Copy link
Member Author

dagar commented Jun 8, 2018

Idea for doing the configuration per module.

  • each module or driver has a set of parameters for each possible instance
    • MAV_PORT_X, MAV_BAUD_X, etc
  • parameters that specify ports have a new serial device metadata tag
    • QGC collects all active serial device parameters and creates a configuration drop down per serial port
  • PX4 side we extend the driver/Device base class to have a serial type prevents duplication
  • each module/driver automatically registers itself for startup (eg mavlink start could be responsible for starting all instances based on MAV_* param configuration)

Does this solution leave anything unresolved?

@stale
Copy link

stale bot commented Jan 29, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@Antiheavy
Copy link
Contributor

this would be a useful feature if implemented!

@stale stale bot removed the Admin: Wont fix label Jan 29, 2019
@hamishwillee
Copy link
Contributor

@Antiheavy This was implemented :-)
Docs are in https://docs.px4.io/en/peripherals/serial_configuration.html

The conflict checks only happen on boot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants