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

RTPS and micro-CDR build system improvements #8084

Merged
merged 1 commit into from
Oct 11, 2017

Conversation

dagar
Copy link
Member

@dagar dagar commented Oct 7, 2017

This PR fixes some build system issues with RTPS and microCDR.

  • serialization moved from each uORB topic header/source to separate files (eg <uORB/topics/sensor_baro.h> -> <uORB_microcdr/topics/sensor_baro.h>)
  • cdr serialization only generated for listed topics
  • micro-CDR library only included and built if RTPS is detected and topics specified
  • micrortps build dependencies fixed

@mcharleb @bkueng any issues with doing this? It improves build times and doesn't burden targets like the px4io with generating and building all this stuff the linker just throws away.

I still don't like the way this quietly depends on fastrtpsgen being found. It creates a situation where users can build and test locally, but break once pushed to CI. If fastrtpsgen isn't yet a hard requirement we should leave it out of the default builds and have rtps configurations where not being able to find fastrtpsgen is a fatal error (nuttx_px4fmu-v4_rtps, posix_sitl_rtps, etc)

This has been split out of #8043.

bkueng
bkueng previously approved these changes Oct 9, 2017
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.

It improves build times and doesn't burden targets like the px4io with generating and building all this stuff the linker just throws away.

I like it!

set(send_topic_files)
foreach(topic ${config_rtps_send_topics})
list(APPEND send_topic_files ${PX4_SOURCE_DIR}/msg/${topic}.msg)
endforeach()
Copy link
Member

Choose a reason for hiding this comment

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

Merge this loop with the next one?

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@@ -0,0 +1,221 @@
@###############################################
Copy link
Member

Choose a reason for hiding this comment

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

is msg/templates/uorb/microRTPS_client.cpp.template still needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

nope, deleted

davids5
davids5 previously approved these changes Oct 9, 2017
Copy link
Member

@davids5 davids5 left a comment

Choose a reason for hiding this comment

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

@dagar I tested this on when it was in #8043. If it is the same set of commits. I am good with it. I have not tested the actual Fast RTPS usages just the build with local support for Fast RTPS generation installed.

@dagar dagar dismissed stale reviews from davids5 and bkueng via 344c200 October 11, 2017 04:39
@dagar dagar added this to the Release v1.7.0 milestone Oct 11, 2017
set(msg_file ${PX4_SOURCE_DIR}/msg/${topic}.msg)
list(APPEND send_topic_files ${msg_file})
get_filename_component(msg ${msg_file} NAME_WE)
list(APPEND uorb_headers_microcdr ${msg_out_path_microcdr}/${msg}.h)
Copy link
Member

Choose a reason for hiding this comment

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

Isn't ${msg} just the same as ${topic}?

Copy link
Member Author

Choose a reason for hiding this comment

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

right

@dagar dagar merged commit 651df03 into PX4:master Oct 11, 2017
@dagar dagar deleted the pr-cmake_rtps_cdr branch October 11, 2017 17:05
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.

3 participants