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

[RFC] Adds the ability to add additional optional files to the ROMFS at compile time #9067

Closed

Conversation

ksschwabe
Copy link
Contributor

@ksschwabe ksschwabe commented Mar 13, 2018

I don't know whether this is a useful feature for the rest of the Px4 community. I find it useful to copy in some "default" files for particular builds into the ROMFS.

When the ROMFS is built/compiled, the files listed in the romfs_optional_files
variable are copied into the ROMFS/px4fmu_common/extras directory within the build directory.

Once the firmware is uploaded, these ROMFS files are present in the etc/extras/ directory
on the Px4 board.

The romfs_optional_files variable is set in the board-specific cmake file (e.g. nuttx_px4fmu-v3_default.cmake). Add the following to the cmake file to add optional extra files to the ROMFS:

set(romfs_optional_files
<path to file 1>
<path to file 2>
<path to file 3>
...
)

@ksschwabe ksschwabe force-pushed the feature_add_optional_files_to_romfs branch 2 times, most recently from 412d721 to b0b489a Compare March 14, 2018 10:39
@@ -4,6 +4,22 @@ message(STATUS "ROMFS: ${config_romfs_root}")
set(romfs_temp_dir ${PX4_BINARY_DIR}/ROMFS/${config_romfs_root})
set(romfs_src_dir ${PX4_SOURCE_DIR}/ROMFS/${config_romfs_root})

# Copy the optional files into ROMFS
message(STATUS "The optional files: ${romfs_optional_files}")
Copy link
Member

Choose a reason for hiding this comment

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

Minor, but could you have this print only when optional files are being included? I'm trying to keep the output as simple as possible so people actually look at it.

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. Will fix. See next version that I push.

)
list(APPEND optional_files ${file_dest})
endforeach()
add_custom_target(collect_optional_extras DEPENDS ${optional_files})
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't the dependency need to be on a custom command OUTPUT to work properly? ${file_dest} in this case.

Copy link
Contributor Author

@ksschwabe ksschwabe Mar 15, 2018

Choose a reason for hiding this comment

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

@dagar. I am not sure... I know that if I don't have the DEPENDS ${optional_files}, then it doesn't include all of the additional files correctly. If I do DEPENDS ${file_dest}, it will only include the last file in the romfs_optional_files list in the ROMFS extra directory. I want the target collect_optional_extras to depend on all of the files in the list romfs_optional_files existing in the ROMFS directory.

From the CMake documentation on add_custom_command, it says the following:

Dependencies listed with the DEPENDS argument may reference files and outputs of custom commands created with add_custom_command() in the same directory (CMakeLists.txt file).

I understood this to mean the DEPENDS argument may "reference files", or it may "reference outputs created with add_custom_command()". I didn't understand it to mean "files created with add_custom_command()". Admittedly, I don't fully understand the ins and outs of CMake, so I could well be wrong.

Copy link
Member

Choose a reason for hiding this comment

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

I found this confusing as well. One thing you can do is generate with ninja, then in the build directory do ninja -v -j1 -d explain. After a full build you can touch an optional file, run the command, then see if the dependencies are working as expected (including actually rebuilding the final binary).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dagar: I think I have now established that my dependencies are not correct. After the first build, if I touch any of my extra files (either in the build directory or in the source directory) then the ROMFS and .px4 file are not being rebuilt.

I have tried for the last few hours to get this to work, but I am just going in circles. :-(

I have noticed that if I touch one of the normal ROMFS source files, e.g. any of the files in Firmware/ROMFS/px4fmu_common/mixers then the ROMFS and the .px4 file is also not rebuilt. I suspect that this is not the desired behaviour.

I also noticed that the variable romfs_src_files (one of the DEPENDS in ROMFS/CMakeLists.txt) isn't set anywhere, so I don't think it is doing anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dagar: Maybe I just needed to sleep on it. I think I have it working correctly now.... :-)

@ksschwabe ksschwabe force-pushed the feature_add_optional_files_to_romfs branch 3 times, most recently from 85349ca to 664922d Compare March 16, 2018 09:06
@ksschwabe
Copy link
Contributor Author

@dagar : I have adjusted the dependencies. I removed the one 'add_custom_command' which wasn't being executed on any subsequent execution of the makefile. Also, the custom command that make copies the init.d files and prunes the ROMFS now depends on the "optional_files" being present in the CMake binary ROMFS directory. If you touch either the "original" additional source file OR the copy in the CMake binary ROMFS directory, then the ROMFS and subsequently the main .px4 file are rebuilt.

Question: Do you think these additional ROMFS files should be pruned, like they currently are? Or should I also modify "px_romfs_pruner.py" to exclude certain files that are passed in as an argument?

…pile time

When the ROMFS is built/compiled, the files listed in the romfs_optional_files
variable are copied into the ROMFS/px4fmu_common/extras directory within the build directory.

Once the firmware is uploaded, these ROMFS files are present in the etc/extras/ directory
on the Px4 board.

The romfs_optional_files variable is set in the board-specific cmake file.
@ksschwabe ksschwabe force-pushed the feature_add_optional_files_to_romfs branch from 664922d to 86ec8ed Compare March 26, 2018 15:37
@dagar
Copy link
Member

dagar commented Mar 26, 2018

One potential pain point here is the pathing. Where are you setting romfs_optional_files? Is it a relative or absolute path? Should we also pull them into the dependencies via px4_add_romfs_files?

@ksschwabe
Copy link
Contributor Author

I was setting romfs_optional_files directly in the master makefile for the board (e.g. nuttx_px4fmu-v3_default), e.g.

px4_nuttx_configure(HWCLASS m4 CONFIG nsh ROMFS y ROMFSROOT px4fmu_common)

set(config_uavcan_num_ifaces 2)

set(romfs_optional_files
	<absolute path>/<additional file 1>
	<absolute path>/<additional file 2>
)

set(config_module_list
	#
	# Board support modules
	#
	drivers/adis16448
	drivers/airspeed
	drivers/blinkm
        ...

I think the path to these extra files has to be the absolute path to work correctly.

These additional files are added to a new list called optional_files which is used as a dependency for generating the ROMFS.

@PX4 PX4 deleted a comment from ksschwabe Mar 27, 2018
@dagar
Copy link
Member

dagar commented Mar 27, 2018

Don't you think we should make it relative if specified within the source? We could probably make it check if either are valid first. We could also use this mechanism for the px4io binary.

@ksschwabe
Copy link
Contributor Author

ksschwabe commented Apr 9, 2018

@dagar: Yes, checking whether the path is relative or absolute makes sense. Where do you envisage that we specify these optional ROMFS files? In the master cmakefile, e.g. nuttx_px4fmu-v3_default.cmake? Or some where else? I think we would need to add a new cmake function, but it would probably have to be in the px4_base.cmake, otherwise the function won't be available when we come to process the master cmake file.

@dagar
Copy link
Member

dagar commented Apr 9, 2018

I'm not sure, maybe we should briefly go over how you'd want to actually use this?

If you're already editing the cmake configuration why isn't this file either part of your fork, or built/fetched by something in your fork (with dependencies)? Are these files optional in the sense that the build should still pass if the optional file you specified isn't found?

@ksschwabe
Copy link
Contributor Author

For me, I'd want the build to fail if the files aren't found. (Perhaps "additional" ROMFS files is a better description, as opposed to "optional" files.)

What I have done until now is have the default ROMFS files, e.g. px4_common, and then each specific board has its own "default" config file that I compile in as an additional ROMFS file. This new file is specific to and different for each board, hence why I don't want to copy it directly into the default ROMFS.

@dagar
Copy link
Member

dagar commented Apr 9, 2018

Ok, well I'm not quite sure what the best way to approach this is overall, but if you find this valuable it looks safe to merge. This is another case where I'd encourage some kind of basic CI testing to prevent it from silently breaking in the future.

@stale
Copy link

stale bot commented Jan 20, 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.

@stale
Copy link

stale bot commented Feb 4, 2019

Closing as stale.

@stale stale bot closed this Feb 4, 2019
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.

2 participants