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

Generate param metadata as MAVLink-compatible JSON component info #15389

Merged
merged 8 commits into from Aug 3, 2020
Merged

Generate param metadata as MAVLink-compatible JSON component info #15389

merged 8 commits into from Aug 3, 2020

Conversation

hamishwillee
Copy link
Contributor

@hamishwillee hamishwillee commented Jul 21, 2020

MAVLink has defined a standardised way for a GCS to query a component's parameter metadata (in this case autopilot) in Using COMPONENT_INFORMATION to obtain vehicle metadata.

This does the first part of that work by automatically generating the parameter metadata in Json format. The generated output for the FIRST version of this can be found here: https://gist.github.com/hamishwillee/3779fcfb6bd368b1cd9fd3f84cbcb5b6

The parser works, but is not ready for merging because the schema defined in mavlink/mavlink#1346 is incomplete/incorrect with respect to the test code provided.
Once schema is signed off by @DonLakeFlyer the output here can be validated/updated. Further modifications for changes should be fairly simple.

Some of the questions I have outstanding:

  • Should every value be defined whether or not empty?
  • Does this also apply to "values"? If values are not defined what should these look like?
  • Note that min value and max value on test and schema mismatch.
  • When is additionalProperties declared, and where?
  • How is "required" used - is this a param property or a higher level property [unclear in schema].

@DonLakeFlyer I have not looked at the translation file - do you have test code for that/workable schema?

FYI @julianoes @dagar I was planning on leaving the next part of this project - the COMPONENT_INFORMATION implementation and working out how to include this file in the Firmware file system for someone else.

Also, does this need to include the param metadata for params that are defined as "injected"?

@hamishwillee
Copy link
Contributor Author

hamishwillee commented Jul 22, 2020

@bkueng How does one go about adding a file to the ROMFS file system (both on nuttx and sitl)? Looking at the code you need to create a CMakeLists.txt and list the files to add in the current directory using px4_add_romfs_files (e.g. see ROMFS/px4fmu_test/init.d/CMakeLists.txt).

  1. Do I need to do anything else - there seem sot be lots of stuff going in within ROMFS/CMakeLists.txt
  2. Where in the file system do these get copied to?
  3. How would you load them - ie is there an example of loading such a file.

The context here is that we have a mavlink message that can be used to return a URL to a file containing parameter metadata. It hasn't been decided where this should live. The easiest/most reliable way is to have it in the Firmware accessed by MAVFTP though of course it might be from a memory size point of view on an HTTP server somewhere instead. If we do that we'd want to generate the file with some unique filename. Do you have any thoughts on that?

But either way, there will be some metadata that we want to host in the firmware, so would be cool to understand this better.

@bkueng
Copy link
Member

bkueng commented Jul 22, 2020

Nice. We're going to need to store this compressed in the ROMFS (and remove optional values to further reduce the size):

 856K px4_parameters.json
  50K px4_parameters.json.bz2
  71K px4_parameters.json.gz
  55K px4_parameters.json.xz
  71K px4_parameters.json.zip

And also send it that way. Is that spec'ed?
I can then help with adding it to the ROMFS.

@hamishwillee
Copy link
Contributor Author

hamishwillee commented Jul 22, 2020

@bkueng

And also send it that way. Is that spec'ed?

It is specced as either plain .json OR a .gz file - the last commit I added generates parameters.json and parameters.json.gz in the same directory.

(and remove optional values to further reduce the size):

The schema was defined but is inconsistent and very slightly incomplete as per description. Point being that if optional values need to be removed I need to know what they are (?) and we need to make sure that the schema reflects that.

We're going to need to store this compressed in the ROMFS

The spec for requesting this file says that it is defined in a URL that can be an http or mavftp. Would it be better to host this on an HTTP server? If so we need a unique URL with less than 70 chars for every param file. FWIW I think ROMFS makes sense for this file, but perhaps the translation files should be on an HTTP server somewhere.

@dagar
Copy link
Member

dagar commented Jul 22, 2020

We should limit this to the subset of parameters included in a particular build (parameter culling). We can also start by including it in the ROMFS except for boards with CONSTRAINED_FLASH set (fmu-v2, etc).

Longer term I also wonder about proper mechanisms for handling the "multi-parameter" cases. Where the parameters and metadata are effectively instances. Things like PWM_MAIN_{MIN, MAX, DIS, FAIL}n, CAL_ACCn, etc. This accounts for a huge amount of duplication in the system currently.

@dagar
Copy link
Member

dagar commented Jul 22, 2020

This is where the parameters.xml is generated using only the modules (and libraries) in a particular build.

https://github.com/PX4/Firmware/blob/01e9599e93775c4e001768528f35e9fa19c6bbee/src/lib/parameters/CMakeLists.txt#L91-L110

Keeping this to a minimum is particularly important if the embedded system is actually going to carry the metadata.

@dagar
Copy link
Member

dagar commented Jul 22, 2020

So if I understand this correctly, we'll carry this in the system wherever possible and Mavlink COMPONENT_INFORMATION (https://mavlink.io/en/messages/common.html#COMPONENT_INFORMATION) will return to the mavlink ftp uri.

Outside of that for the flash constrained boards we can have an http backup. Perhaps we could just dump these on s3 accessible with a specified hash on some stable output of the parameter generation?

@hamishwillee
Copy link
Contributor Author

I have updated this to follow the xml script. So what this means is that it only includes the optional parameters if they are defined. Don, this adds Bitmask, which was not in your schema.

@dagar

  1. Can you make, or show me how to make "exactly" the changes you suggest to build this for only used modules in Generate param metadata as MAVLink-compatible JSON component info #15389 (comment). I have updated the json parser so it can take the injected XML and board as parameters [though actual plumbing for those not yet implemented] so this should just be a CMakeLists.txt change. I'd do it myself except I tried, and the parser wasn't called.
  2. Is there any example of board-specific params? I'm not certain my code works as expected for these and I can't find any by searching, so wondered if perhaps they are theoretical.

So if I understand this correctly, we'll carry this in the system wherever possible and Mavlink COMPONENT_INFORMATION (https://mavlink.io/en/messages/common.html#COMPONENT_INFORMATION) will return to the mavlink ftp uri.
Outside of that for the flash constrained boards we can have an http backup. Perhaps we could just dump these on s3 accessible with a specified hash on some stable output of the parameter generation?

That is correct - the COMPONENT_INFORMATION uri can either be mavftp or http URL. If it is HTTP it really needs to be unique.

@DonLakeFlyer do you already have a mechanism for dealing with the case where metadata is common for a number of parameter instances? As raised by Dan here: #15389 (comment)

@bkueng
Copy link
Member

bkueng commented Jul 23, 2020

It is specced as either plain .json OR a .gz file

Can we add bz2 or lzma? The more we can bring it down the better.

Overall I see 3 different ways:

  • keep in ROMFS (works best for custom builds)
  • keep on a server - we need the version in the URL
  • keep in the .px4 file and the update will push it to the SD card / companion (mostly interesting if we want to add translations as well, in particular for custom builds)

As Daniel pointed out, this is the target-specific cmake target to currently build the xml file: https://github.com/PX4/Firmware/blob/01e9599e93775c4e001768528f35e9fa19c6bbee/src/lib/parameters/CMakeLists.txt#L94
You can e.g. build the omnibus target, and the build directory will contain the xml w/o the FW_ params.

@hamishwillee
Copy link
Contributor Author

@bkueng

Can we add bz2 or lzma? The more we can bring it down the better.

We settled on .gz because it was already supported by PX4 and the same platforms as QGC/MAVSDK. If there are convenient libraries for doing the same encryption/decryption using these other formats then I don't see why not. But we want to settle on something and stick to it for everything. @julianoes - any objections?

As Daniel pointed out, this is the target-specific cmake target to currently build the xml file: ....

Yes. As far as I can tell this changes what is supplied to the parser, so should also work for the json output as here. Unfortunately when I tried to modify this to also build the json it did not work - didn't fail, just didn't appear to generate the json file.

Since it looked trivial to copy/modify I can only assume that there is more to it than I thought - so would appreciate it if someone else did this bit.

FYI, I also added #15403 so that json and docs can also include the injected params.

@bkueng
Copy link
Member

bkueng commented Jul 24, 2020

I guess you were missing a dependency then. This would generate it:

diff --git a/src/lib/parameters/CMakeLists.txt b/src/lib/parameters/CMakeLists.txt
--- a/src/lib/parameters/CMakeLists.txt
+++ b/src/lib/parameters/CMakeLists.txt
@@ -109,12 +109,35 @@ add_custom_command(OUTPUT ${parameters_xml}
 )
 add_custom_target(parameters_xml DEPENDS ${parameters_xml})
 
+set(parameters_json ${PX4_BINARY_DIR}/parameters.json)
+add_custom_command(OUTPUT ${parameters_json}
+	COMMAND ${PYTHON_EXECUTABLE} ${CMAKE_CURRENT_SOURCE_DIR}/px_process_params.py
+		--src-path ${module_list} ${generated_params_dir}
+		--json ${parameters_json}
+		--inject-xml ${CMAKE_CURRENT_SOURCE_DIR}/parameters_injected.xml
+		--overrides ${PARAM_DEFAULT_OVERRIDES}
+		--board ${PX4_BOARD}
+		#--verbose
+	DEPENDS
+		${param_src_files}
+		${generated_serial_params_file}
+		parameters_injected.xml
+		px4params/srcparser.py
+		px4params/srcscanner.py
+		px4params/xmlout.py
+		px_process_params.py
+		parameters_injected.xml
+	COMMENT "Generating parameters.json"
+)
+add_custom_target(parameters_json DEPENDS ${parameters_json})
+
 # generate px4_parameters.c and px4_parameters{,_public}.h
 add_custom_command(OUTPUT px4_parameters.c px4_parameters.h px4_parameters_public.h
 	COMMAND ${PYTHON_EXECUTABLE} ${CMAKE_CURRENT_SOURCE_DIR}/px_generate_params.py
 		--xml ${parameters_xml} --dest ${CMAKE_CURRENT_BINARY_DIR}
 	DEPENDS
 		${PX4_BINARY_DIR}/parameters.xml
+		${PX4_BINARY_DIR}/parameters.json
 		px_generate_params.py
 		templates/px4_parameters.c.jinja
 		templates/px4_parameters.h.jinja

I can add it to the romfs later.

@DonLakeFlyer
Copy link
Contributor

FYI: mavlink/mavlink#1431 This should be good schema definitions you can use for validation.

@hamishwillee
Copy link
Contributor Author

@DonLakeFlyer

  1. Thanks. That Schema much better. I have added a few suggestions.
  2. From Beat "Can we add bz2 or lzma? The more we can bring it down the better." Ie if you are OK with this I will try find a library to use one of these instead of .gz.

@bkueng Thanks for that CMakeLists fixed up as you suggested and works.

I can add it to the romfs later.

Would it be possible to add it to SITL file system first/early as this would allow us to do testing before it appears in Firmware?

@DonLakeFlyer
Copy link
Contributor

2. From Beat "Can we add bz2 or lzma? The more we can bring it down the better." Ie if you are OK with this I will try find a library to use one of these instead of .gz.

I'd rather stick with gzip since it's simple from a standpoint of cross-platform code support and well as multi-language support (C, Python, ...). Also gzip is the underlying basis for zip which at some point would be useful for QGC to support in order to decompress .kmz files. QGC also uses gzip for decompressing ArduPilot firmware manifest files. The farther you go down the road of more complex compression algorithms the harder it gets for QGC to deal with cross-platform/language.

@hamishwillee
Copy link
Contributor Author

  1. From Beat "Can we add bz2 or lzma? The more we can bring it down the better." Ie if you are OK with this I will try find a library to use one of these instead of .gz.

I'd rather stick with gzip since it's simple from a standpoint of cross-platform code support and well as multi-language support (C, Python, ...). ... The farther you go down the road of more complex compression algorithms the harder it gets for QGC to deal with cross-platform/language.

@bkueng This is why we picked ".gz" in the first place. Any thoughts on this/experience with cross platform encryption libraries. The 7-Zip SDK looks promising, but of course not as easy as just sticking with what we have got.

@DonLakeFlyer My concern is that not doing this this will make it harder for embedded systems to choose to use this feature.

@DonLakeFlyer
Copy link
Contributor

My suggestion is to move forward with gzip for now, I can look into whether I can get bz2 to work. But that is going to take some time since getting this stuff to work on all platforms is always painful. I don't see a reason to hold the whole thing up on that decision.

My concern is that not doing this this will make it harder for embedded systems to choose to use this feature.

Why? If there isn't enough room to store it on the board you put it on the internet. Both cases need to be supported.

@hamishwillee
Copy link
Contributor Author

@DonLakeFlyer I agree, we can push forward with this. You're probably right that the Internet will be the best path for people who don't want the overhead. For a start, it avoids them even having to think about mavftp.

@bkueng Can you please merge this? It complies with the schema and generates both injected and build-specific params.
Then, can we get this into firmware - at least on SITL for now? I can add the bit to stream it.

Copy link
Member

@bkueng bkueng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok gz it is for now, but I still prefer to have either bz2 or lzma as they're generally superior. I know compiling on different platforms is tedious, but these libs should be fairly OS-independent, so it should not be a big problem.

add_custom_command(OUTPUT ${parameters_json}
COMMAND ${PYTHON_EXECUTABLE} ${CMAKE_CURRENT_SOURCE_DIR}/px_process_params.py
--src-path ${module_list} ${generated_params_dir}
--json ${parameters_json}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we generate both json + xml in the same python call? I.e. merge this with the above.

# generate px4_parameters.c and px4_parameters{,_public}.h
add_custom_command(OUTPUT px4_parameters.c px4_parameters.h px4_parameters_public.h
COMMAND ${PYTHON_EXECUTABLE} ${CMAKE_CURRENT_SOURCE_DIR}/px_generate_params.py
--xml ${parameters_xml} --dest ${CMAKE_CURRENT_BINARY_DIR}
DEPENDS
${PX4_BINARY_DIR}/parameters.xml
${PX4_BINARY_DIR}/parameters.json
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only added this for testing, we're going to need to add the dependency at the right place.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. Since I don't know what I'm doing, can you make the needed change?

Copy link
Member

@bkueng bkueng Jul 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here it is: bkueng@6349aea. I also made the compression generic.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @bkueng . I've added that. What next?

src/lib/parameters/px4params/jsonout.py Outdated Show resolved Hide resolved
@DonLakeFlyer
Copy link
Contributor

but these libs should be fairly OS-independent, so it should not be a big problem.

I did some initial poking around on bz2. Shouldn't be too hard I hope.

@hamishwillee
Copy link
Contributor Author

but these libs should be fairly OS-independent, so it should not be a big problem.

I did some initial poking around on bz2. Shouldn't be too hard I hope.

@DonLakeFlyer @bkueng Good, because IMO settling on one format matters a lot. If we say either .bz2 or .gz can be used then we're also saying any GCS that wants to support this needs to support both formats. That makes things harder. So I hope you can get .bz2 working and that would become the format we state.

@bkueng bkueng merged commit 958d5a3 into PX4:master Aug 3, 2020
@hamishwillee hamishwillee deleted the json_param_metadata branch August 3, 2020 23:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants