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

PlotJuggler with Fast-CDR-2.x.x #932

Merged
merged 6 commits into from
Feb 20, 2024

Conversation

manuelValch
Copy link
Contributor

Resolve: #919 #861

Reworked previous MR:

  • removed cmake update to maintain compatibility with all version
  • udpated cmake so that build does not fail if Fast-Cdr is found on dev PC

Added message if FastCdr is found on local PC:

-- [FastCdr] found, version: 2.1.3

If not found:

-- [FastCdr] not found, create shared libraries

Tests

Tried to load some csv under datasamples/rosbag2_test/:

image

Also able to stream data from fastDDS using eProsima plugin:

plotjugDDS

I can not test on old ros version but I hope it should be fixed now.

@manuelValch manuelValch closed this Feb 6, 2024
@manuelValch manuelValch reopened this Feb 6, 2024
@manuelValch
Copy link
Contributor Author

After last commit, able to compile and run with fastCdr 1.1.1:
image

Comment on lines +24 to +32
switch (cdr_version_)
{
case CdrVersion::XCDRv2:
break;
default:
current_encoding_ = EncodingAlgorithmFlag::PLAIN_CDR;
align64_ = 8;
break;
}

Check notice

Code scanning / CodeQL

No trivial switch statements Note library

This switch statement should either handle more cases, or be rewritten as an if statement.
}
else
{
extra_size = 4; // EMHEADER;

Check notice

Code scanning / CodeQL

Commented-out code Note library

This comment appears to contain commented-out code.
}
else
{
extra_size = 4; // EMHEADER;

Check notice

Code scanning / CodeQL

Commented-out code Note library

This comment appears to contain commented-out code.
if (CdrVersion::XCDRv2 == cdr_version_ &&
EncodingAlgorithmFlag::PL_CDR2 != current_encoding_)
{
// Take into account the boolean is_present;

Check notice

Code scanning / CodeQL

Commented-out code Note library

This comment appears to contain commented-out code.
@manuelValch manuelValch force-pushed the 919-build-fastcdr-2.x.x branch 2 times, most recently from e5b3db7 to 4e78ee9 Compare February 6, 2024 21:46
Comment on lines 12 to 20
find_package(fastcdr QUIET)

if(NOT fastcdr_FOUND )
# Override Fast-CDR option: compile as static lib
SET(BUILD_SHARED_LIBS OFF CACHE BOOL "Create shared libraries by default")
add_subdirectory(3rdparty/Fast-CDR)
include_directories(3rdparty/Fast-CDR/include)
endif()

Copy link
Owner

Choose a reason for hiding this comment

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

I don't understand. If we are not compiling the library in 3rd party, how this even work?

Copy link
Contributor Author

@manuelValch manuelValch Feb 8, 2024

Choose a reason for hiding this comment

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

I moved it to the upper makefile:
plotjuggler_plugins/ParserROS/CMakeLists.txt

My guess is that for some reason cmake does not propagate fastcdr to ParserRos CMakeLists.
And because ROSparser needs 3rdparty lib to link it tris to find fastCdr AT ROSParser level and fails.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe I could just move the find haven't tried yet.
I can test, but I won't be able to do it before next week

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, I believe you should move that code back to the other Cmake, thanks!!!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved back 3rdparty build where it was.
had to leave find(fastcdr QUIET) though otherwise build would fail with fastcdr installed on build machine i.e
this issue: #861

Also rebased on latest main which is why (I think) windows CI does not pass.

manuelValch and others added 6 commits February 14, 2024 21:23
If this commit is applied then plotjuggler will be able to compile and
run with Fast-CDR version 2.x.x from eProsima.
It can compile from:
- local thirdparty folder included within the repo
- install of fastcdr within the machine (usually /usr/local)

to do so:
- Udpated files, API and namespace names
- Udpate configure.ac to match newly updated version

Done it because I need to use plotJuggler with latest lib from eProsima
and I would like this to be available for everyone.

Issue:
[facontidavide#919]
Added missing files from Fastcdr Ci shall build.
PlotJuggler build again with or without fastcdr package installed.

Issue:
[facontidavide#919]
Moved up findCdr or build default library so that cmake does not fail
before building when fastCdr is installed on system.
Added message so that user knows when fastCdr is found on its system.

Issue:
[facontidavide#919]
Co-authored-by: manuelValch
Remain compatible to fastCdr version 1.1.x so that
build with Ros is not broken

Issue:
[facontidavide#919]
Co-authored-by: manuelValch
As I love those splash screen tried to make a meme for it.
Do not hesitate to drop it if you don't want it.
Updated main and ressource to find the file

Co-authored-by: ManuelValch <[email protected]>
If this commit is applied then fastCDR library will be build in original
CMakeLists.txt file i.e ./rosx_introspection.

Done that as it is a change requested in pull request.

Issue:
[facontidavide#919]
Co-authored-by: ManuelValch <[email protected]>
@facontidavide facontidavide merged commit 5493e7a into facontidavide:main Feb 20, 2024
10 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PlotJuggler does not build with fast-CDR version 2.x.x
2 participants