ROS2 Point One Nav Atlas Driver

[sisaha9]

Building on @jimenezjose 's ROS2 port I have made some changes and verified it worked here: https://github.com/airacingtech/fusion-engine-client/tree/sid/ros2

I made some changes to make it a more ready ROS2 build as the previous version did not fit in a workspace as well. I made the UDP client and TCP client testers ROS2 nodes and added options in the Atlas driver to run on either UDP or TCP connection (former added by @jimenezjose ). It publishes GPSFix, IMU, NavSatFix and PoseStamped messages (former 2 added by @jimenezjose )

If the Fusion Engine Client can be made into an apt package that can be included instead of having to include it in the repo. That will keep the ROS2 package more or less in sync with latest changes without having to rebase constantly. I also see @jimenezjose 's fork has some more changes. If we can find a way to integrate these 2 that will be ideal (Which is why this is an Issue and not a PR)

[adamshapiro0]

Hi @sisaha9, glad to have some help on this! None of us are ROS experts, so having some input helps a lot.

I'm a little confused about the design here. It looks like all of the C++ and Python FusionEngine code has been removed and replaced with ROS code, plus just a few files from the FusionEngine C++ code. Is this meant to be a separate repository? I think @jimenezjose may have mentioned that ROS expects the ROS code to be at the top level of the driver repo, is that correct and why it is structured this way?

Assuming the ROS driver would be a separate repository, regarding the FusionEngine client code duplication, is there any reason not to use CMake FetchContent or ExternalProject to pull in the code from https://github.com/PointOneNav/fusion-engine-client? Or alternatively, use git submodules to do it? Submodules is probably less ideal since it requires the user to do an extra step to setup/update the submodule when they build, but still better than copying the code.

I strongly suggest avoiding duplicating any code since it's much harder to trace and will quickly become out of date as the upstream source code is modified and improved.

[sisaha9]

@adamshapiro0 @jimenezjose I have made another repo in which the fusion engine client is a submodule instead of being a subfolder of the driver: https://github.com/airacingtech/ros2-atlas-driver/tree/stable

I removed the test TCP and UDP nodes as the fusion-engine-client does not have the print_message file right now. Regardless it will still be accessible in the fusion-engine-client. I tested this with both TCP and UDP connection and it worked

Additionally, there is another submodule to a heading generator node that can subscribe to 2 different units and find heading in ENU frame

[jimenezjose]

@adamshapiro0

Are you aware of these [-Wpedantic] warnings?
Files: (configuration.h, control.h)

Starting >>> atlas_driver
Starting >>> car_heading_generator
[Processing: atlas_driver, car_heading_generator]                                                            
--- stderr: atlas_driver                                                                                      
In file included from /.../atlas_driver/include/point_one/fusion_engine/messages/core.h:8,
                 from /.../atlas_driver/include/atlas_driver/atlas.hpp:8,
                 from /.../atlas_driver/src/atlas_node.cpp:17:
/... /atlas_driver/include/point_one/fusion_engine/messages/configuration.h:293:31: warning: ISO C++ forbids zero-size array ‘config_change_data’ [-Wpedantic]
   uint8_t config_change_data[0];
                               ^
/.../atlas_driver/include/point_one/fusion_engine/messages/configuration.h:386:31: warning: ISO C++ forbids zero-size array ‘config_change_data’ [-Wpedantic]
   uint8_t config_change_data[0];
                               ^
/.../atlas_driver/include/point_one/fusion_engine/messages/configuration.h:1131:27: warning: ISO C++ forbids zero-size array ‘stream_indices’ [-Wpedantic]
   uint8_t stream_indices[0];
                           ^
/.../atlas_driver/include/point_one/fusion_engine/messages/configuration.h:1163:30: warning: invalid use of ‘struct point_one::fusion_engine::messages::OutputInterfaceConfigEntry’ with a zero-size array in ‘struct point_one::fusion_engine::messages::SetOutputInterfaceConfigMessage’ [-Wpedantic]
   OutputInterfaceConfigEntry output_interface_data;
                              ^~~~~~~~~~~~~~~~~~~~~
/.../atlas_driver/include/point_one/fusion_engine/messages/configuration.h:1131:27: note: array member ‘uint8_t point_one::fusion_engine::messages::OutputInterfaceConfigEntry::stream_indices [0]’ declared here
   uint8_t stream_indices[0];
                           ^
In file included from /.../atlas_driver/include/point_one/fusion_engine/messages/core.h:9,
                 from /.../atlas_driver/include/atlas_driver/atlas.hpp:8,
                 from /.../atlas_driver/src/atlas_node.cpp:17:
/.../atlas_driver/include/point_one/fusion_engine/messages/control.h:307:24: warning: ISO C++ forbids zero-size array ‘fw_version_str’ [-Wpedantic]
   char fw_version_str[0];
                        ^
/.../atlas_driver/include/point_one/fusion_engine/messages/control.h:362:28: warning: ISO C++ forbids zero-size array ‘event_description’ [-Wpedantic]
   char* event_description[0];

[adamshapiro0]

Hi @sisaha9, thanks for creating that repo! We haven't had a chance to look it over just yet, but we're excited to have it and will be sure to give feedback soon!

Would it be possible to rename "Atlas" with "FusionEngine" (or fusion_engine) in the repo name and the code so it matches this repo? The FE protocol actually works across multiple Point One device models, Atlas is just one of the models.

[adamshapiro0]

@jimenezjose thanks for pointing those out. What version of GCC (or other compiler) are you using that is generating those warnings? We don't see those with ours currently -- we test against GCC 9.4, clang 11.0, and MSVC 19.32.

Reading https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html more closely, it looks like it may still be worth addressing even without the warnings. In theory, those members are just meant to be convenient pointers, and should not change the size of the struct itself. According to the GCC documentation though, they may (it's not guaranteed):

Although the size of a zero-length array is zero, an array member of this kind may increase the size of the enclosing type as a result of tail padding.

@axlan we should look at this a bit closer ^.

[jimenezjose]

@adamshapiro I am using the GNU 8.4.0 compiler. For completeness here is the CMakeCXXCompiler.cmake generated from the cmake binary output.

set(CMAKE_CXX_COMPILER "/usr/bin/c++")
set(CMAKE_CXX_COMPILER_ARG1 "")
set(CMAKE_CXX_COMPILER_ID "GNU")
set(CMAKE_CXX_COMPILER_VERSION "8.4.0")
set(CMAKE_CXX_COMPILER_VERSION_INTERNAL "")
set(CMAKE_CXX_COMPILER_WRAPPER "")
set(CMAKE_CXX_STANDARD_COMPUTED_DEFAULT "14")
set(CMAKE_CXX_COMPILE_FEATURES "cxx_std_98;cxx_template_template_parameters;cxx_std_11;cxx_alias_templates;cxx_alignas;cxx_alignof;cxx_attributes;cxx_auto_type;cxx_constexpr;cxx_decltype;cxx_decltype_incomplete_return_types;cxx_default_function_template_args;cxx_defaulted_functions;cxx_defaulted_move_initializers;cxx_delegating_constructors;cxx_deleted_functions;cxx_enum_forward_declarations;cxx_explicit_conversions;cxx_extended_friend_declarations;cxx_extern_templates;cxx_final;cxx_func_identifier;cxx_generalized_initializers;cxx_inheriting_constructors;cxx_inline_namespaces;cxx_lambdas;cxx_local_type_template_args;cxx_long_long_type;cxx_noexcept;cxx_nonstatic_member_init;cxx_nullptr;cxx_override;cxx_range_for;cxx_raw_string_literals;cxx_reference_qualified_functions;cxx_right_angle_brackets;cxx_rvalue_references;cxx_sizeof_member;cxx_static_assert;cxx_strong_enums;cxx_thread_local;cxx_trailing_return_types;cxx_unicode_literals;cxx_uniform_initialization;cxx_unrestricted_unions;cxx_user_literals;cxx_variadic_macros;cxx_variadic_templates;cxx_std_14;cxx_aggregate_default_initializers;cxx_attribute_deprecated;cxx_binary_literals;cxx_contextual_conversions;cxx_decltype_auto;cxx_digit_separators;cxx_generic_lambdas;cxx_lambda_init_captures;cxx_relaxed_constexpr;cxx_return_type_deduction;cxx_variable_templates;cxx_std_17;cxx_std_20")
set(CMAKE_CXX98_COMPILE_FEATURES "cxx_std_98;cxx_template_template_parameters")
set(CMAKE_CXX11_COMPILE_FEATURES "cxx_std_11;cxx_alias_templates;cxx_alignas;cxx_alignof;cxx_attributes;cxx_auto_type;cxx_constexpr;cxx_decltype;cxx_decltype_incomplete_return_types;cxx_default_function_template_args;cxx_defaulted_functions;cxx_defaulted_move_initializers;cxx_delegating_constructors;cxx_deleted_functions;cxx_enum_forward_declarations;cxx_explicit_conversions;cxx_extended_friend_declarations;cxx_extern_templates;cxx_final;cxx_func_identifier;cxx_generalized_initializers;cxx_inheriting_constructors;cxx_inline_namespaces;cxx_lambdas;cxx_local_type_template_args;cxx_long_long_type;cxx_noexcept;cxx_nonstatic_member_init;cxx_nullptr;cxx_override;cxx_range_for;cxx_raw_string_literals;cxx_reference_qualified_functions;cxx_right_angle_brackets;cxx_rvalue_references;cxx_sizeof_member;cxx_static_assert;cxx_strong_enums;cxx_thread_local;cxx_trailing_return_types;cxx_unicode_literals;cxx_uniform_initialization;cxx_unrestricted_unions;cxx_user_literals;cxx_variadic_macros;cxx_variadic_templates")
set(CMAKE_CXX14_COMPILE_FEATURES "cxx_std_14;cxx_aggregate_default_initializers;cxx_attribute_deprecated;cxx_binary_literals;cxx_contextual_conversions;cxx_decltype_auto;cxx_digit_separators;cxx_generic_lambdas;cxx_lambda_init_captures;cxx_relaxed_constexpr;cxx_return_type_deduction;cxx_variable_templates")
set(CMAKE_CXX17_COMPILE_FEATURES "cxx_std_17")
set(CMAKE_CXX20_COMPILE_FEATURES "cxx_std_20")

set(CMAKE_CXX_PLATFORM_ID "Linux")
set(CMAKE_CXX_SIMULATE_ID "")
set(CMAKE_CXX_COMPILER_FRONTEND_VARIANT "")
set(CMAKE_CXX_SIMULATE_VERSION "")



set(CMAKE_AR "/usr/bin/ar")
set(CMAKE_CXX_COMPILER_AR "/usr/bin/gcc-ar-8")
set(CMAKE_RANLIB "/usr/bin/ranlib")
set(CMAKE_CXX_COMPILER_RANLIB "/usr/bin/gcc-ranlib-8")
set(CMAKE_LINKER "/usr/bin/ld")
set(CMAKE_MT "")
set(CMAKE_COMPILER_IS_GNUCXX 1)
set(CMAKE_CXX_COMPILER_LOADED 1)
set(CMAKE_CXX_COMPILER_WORKS TRUE)
set(CMAKE_CXX_ABI_COMPILED TRUE)
set(CMAKE_COMPILER_IS_MINGW )
set(CMAKE_COMPILER_IS_CYGWIN )
if(CMAKE_COMPILER_IS_CYGWIN)
  set(CYGWIN 1)
  set(UNIX 1)
endif()

set(CMAKE_CXX_COMPILER_ENV_VAR "CXX")

if(CMAKE_COMPILER_IS_MINGW)
  set(MINGW 1)
endif()
set(CMAKE_CXX_COMPILER_ID_RUN 1)
set(CMAKE_CXX_SOURCE_FILE_EXTENSIONS C;M;c++;cc;cpp;cxx;m;mm;CPP)
set(CMAKE_CXX_IGNORE_EXTENSIONS inl;h;hpp;HPP;H;o;O;obj;OBJ;def;DEF;rc;RC)

foreach (lang C OBJC OBJCXX)
  if (CMAKE_${lang}_COMPILER_ID_RUN)
    foreach(extension IN LISTS CMAKE_${lang}_SOURCE_FILE_EXTENSIONS)
      list(REMOVE_ITEM CMAKE_CXX_SOURCE_FILE_EXTENSIONS ${extension})
    endforeach()
  endif()
endforeach()

set(CMAKE_CXX_LINKER_PREFERENCE 30)
set(CMAKE_CXX_LINKER_PREFERENCE_PROPAGATES 1)

# Save compiler ABI information.
set(CMAKE_CXX_SIZEOF_DATA_PTR "8")
set(CMAKE_CXX_COMPILER_ABI "ELF")
set(CMAKE_CXX_LIBRARY_ARCHITECTURE "aarch64-linux-gnu")

if(CMAKE_CXX_SIZEOF_DATA_PTR)
  set(CMAKE_SIZEOF_VOID_P "${CMAKE_CXX_SIZEOF_DATA_PTR}")
endif()

if(CMAKE_CXX_COMPILER_ABI)
  set(CMAKE_INTERNAL_PLATFORM_ABI "${CMAKE_CXX_COMPILER_ABI}")
endif()

if(CMAKE_CXX_LIBRARY_ARCHITECTURE)
  set(CMAKE_LIBRARY_ARCHITECTURE "aarch64-linux-gnu")
endif()

set(CMAKE_CXX_CL_SHOWINCLUDES_PREFIX "")
if(CMAKE_CXX_CL_SHOWINCLUDES_PREFIX)
  set(CMAKE_CL_SHOWINCLUDES_PREFIX "${CMAKE_CXX_CL_SHOWINCLUDES_PREFIX}")
endif()





set(CMAKE_CXX_IMPLICIT_INCLUDE_DIRECTORIES "/usr/include/c++/8;/usr/include/aarch64-linux-gnu/c++/8;/usr/include/c++/8/backward;/usr/lib/gcc/aarch64-linux-gnu/8/include;/usr/local/include;/usr/include/aarch64-linux-gnu;/usr/include")
set(CMAKE_CXX_IMPLICIT_LINK_LIBRARIES "stdc++;m;gcc_s;gcc;c;gcc_s;gcc")
set(CMAKE_CXX_IMPLICIT_LINK_DIRECTORIES "/usr/lib/gcc/aarch64-linux-gnu/8;/usr/lib/aarch64-linux-gnu;/usr/lib;/lib/aarch64-linux-gnu;/lib")
set(CMAKE_CXX_IMPLICIT_LINK_FRAMEWORK_DIRECTORIES "")

[jimenezjose]

@adamshapiro0 We have been a doing a lot of great work here in hawaii on an autonomous go kart (codename pip - project in paradise) using two of your Atlas units. The ROS2 driver for Point One Navigation GNSS/INS devices is about finalized. I'd like your input to begin making this an official driver. I also decoupled your fusion engine library from the driver such that in my CMakeLists.txt I use the ExternalProject_Add function to create a custom target depending on your fusion_engine git repository. I then simply add a dynamic linking runtime parameter to add the fusion_engine shared library.

[adamshapiro0]

Hi Jose,

That's great to hear. We're definitely excited to have ROS driver support, and really appreciate the contribution!

ExternalProject_Add sounds like a good way to bring in the library within cmake instead of using a git submodule. Either that or FetchContent. They both have pros and cons, we've used them both at various times.

I looked it over and overall it looks pretty good! I'm not super familiar with the ROS stuff, but I'll assume that is working and implemented the way it's supposed to be?

A couple small comments on the current stuff:

• Any objection to changing the name to fusion_engine_ros_driver? For consistency, we like to use FusionEngine since it's the name of the protocol, vs Atlas (just one of our devices) or GPS (just one of its capabilities)
  • You'd also likely want to change the PointOneGps class name and its documentation similarly. Depending on what you pick for that, you might want to modify the name of your FusionEngineClient class so it's clear which one is the main entry point for the driver (the PointOneGps class)
• Looks like PointOneGps and FusionEngineClient have the same class description, probably just copy/paste error
• Does ROS require the code to be in the point_one_gps_driver subdirectory, or would it work if src/ and everything was at the top of the repo?
• How would you feel changing the code style a bit to match our other repos just for consistency? Looks like just some small things. The main changes would be to:
  • Apply our .clang-format file
  • Change function names to MyFunc() instead of myFunc()
  • Change variable names to lowercase + underscores, with a trailing _ for member variables (listener_list_ instead of listenerList)
  • Use @brief for the first sentence of class and function Doxygen descriptions
  • Use /** for class descriptions, similar to function descriptions. That way Doxygen would pick them up if it was run on the code (Doxygen ignores /* comment blocks)

Let's knock these things out whenever you get some free time, and then we can look at setting up an official repo.