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

Jett Onboarding New #97

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 72 additions & 0 deletions 10_Examples/jett-onboarding/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
cmake_minimum_required(VERSION 3.8)
project(jett-onboarding)

if(CMAKE_COMPILER_IS_GNUCXX OR CMAKE_CXX_COMPILER_ID MATCHES "Clang")
add_compile_options(-Wall -Wextra -Wpedantic)
endif()

# find dependencies
find_package(ament_cmake REQUIRED)
find_package(rclcpp REQUIRED)
find_package(std_msgs REQUIRED)

find_package(ament_cmake_gtest REQUIRED)

# uncomment the following section in order to fill in
# further dependencies manually.
# find_package(<dependency> REQUIRED)
add_executable(listener src/subscriber_member_function.cpp)
ament_target_dependencies(listener rclcpp std_msgs)

add_executable(talker src/publisher_member_function.cpp)
ament_target_dependencies(talker rclcpp std_msgs)
install(TARGETS
talker
listener
DESTINATION lib/${PROJECT_NAME})
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would either have separate install(TARGETS... for each target (target being an executable or library), or have one big one at the very bottom of the file.

Doesnt actually matter in this case, just for readability. A lot of times we can get away with pattern matching CMake stuff off a working project, but it is good to treat it the same way you would regular code (meaning, make it clean and readable for others/future you).


add_executable(hello_world src/hello_world.cpp)
install(TARGETS
hello_world
DESTINATION lib/${PROJECT_NAME})
target_link_libraries(hello_world
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could be quirky that this line is below the install. Again, if it works its fine, but usually I would install last.

If you want more in-depth info on what any of the CMake stuff does, I would see comments in ghost_examples CMake which is supposed to be gold standard.

talking
)

# Add Class as Library to allow it to share across files easier probably
add_library(talking SHARED src/talking.cpp)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Fun fact time:

There are mainly two types of libraries: Dynamic/Shared and Static.

When you link a static library to another target (lib or exe), it basically copy pasts all the cpp files into that same executable.

When you link a shared library, the other program is gonna look for 1's and 0's for that library with you actually run the program. So, if we built everything, and then someone went in and deleted my_shared_library.so, then any exes or libs that depended on it would break when you ran them.

Not gonna go into why you would choose one or the other, but good to have heard of the concept before.

ament_target_dependencies(talking
${DEPENDENCIES}
)
ament_export_targets(talking HAS_LIBRARY_TARGET)
install(
TARGETS talking
EXPORT talking
LIBRARY DESTINATION lib
ARCHIVE DESTINATION lib
RUNTIME DESTINATION bin
INCLUDES DESTINATION include
)


ament_add_gtest(testFile src/testFile.cpp)
target_link_libraries(testFile
talking
gtest_main
)

# if(BUILD_TESTING)
# find_package(ament_lint_auto REQUIRED)

# # the following line skips the linter which checks for copyrights
# # comment the line when a copyright and license is added to all source files
# set(ament_cmake_copyright_FOUND TRUE)

# # the following line skips cpplint (only works in a git repo)
# # comment the line when this package is in a git repo and when
# # a copyright and license is added to all source files
# set(ament_cmake_cpplint_FOUND TRUE)
# ament_lint_auto_find_test_dependencies()
# endif()

ament_package()
21 changes: 21 additions & 0 deletions 10_Examples/jett-onboarding/package.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
<?xml version="1.0"?>
<?xml-model href="http://download.ros.org/schema/package_format3.xsd" schematypens="http://www.w3.org/2001/XMLSchema"?>
<package format="3">
<name>jett-onboarding</name>
<version>0.0.0</version>
<description>TODO: Package description</description>
<maintainer email="[email protected]">jettvanbrocklin</maintainer>
<license>TODO: License declaration</license>

Copy link
Collaborator

Choose a reason for hiding this comment

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

Doesnt matter for this onboarding, but generally its good to fill in description and license as well when you add new package for the first time (because otherwise you are never gonna go back and fill them in lol).

<buildtool_depend>ament_cmake</buildtool_depend>

<depend>rclcpp</depend>
<depend>std_msgs</depend>

Copy link
Collaborator

Choose a reason for hiding this comment

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

So, calling that find_package up in the CMake is the primary way that we list dependencies and we would link to libraries in a different package, but we list them here.

The main reason for listing them here, is if you have packages that depend on each other within the same ROS workspace (so for VEXU, thats just our whole repo), listing dependency tells colcon what order to build things in.

So, if pkg A lists pkg B as a dependency here, then when you build the full repo, it wont start compiling A until B is totally done. Otherwise A will look for stuff that doesnt exist yet, and fail.

<test_depend>ament_lint_auto</test_depend>
<test_depend>ament_lint_common</test_depend>

<export>
<build_type>ament_cmake</build_type>
</export>
</package>
14 changes: 14 additions & 0 deletions 10_Examples/jett-onboarding/src/hello_world.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
#include "talking.h"
#include <iostream>
#include "string.h"

using namespace std;

int test();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I assume this is an artifact from before migrating to separate test file. Thats no problem really, but good opportunity for learning stuff.

In this case, we have "declared" a function, but have not yet "defined" it. Basically, you could try to call this test function in your main, and this declaration is like a promise to the rest of the code that we will define it later. I bring it up because if you DONT define it later, you will get confusing errors that might not happen until you try to run it (i.e. it might compile fine, depends on situation).


int main(int argc, char ** argv)
{
talking t1;
t1.talk("Hello World");
return 0;
}
56 changes: 56 additions & 0 deletions 10_Examples/jett-onboarding/src/publisher_member_function.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
// Copyright 2016 Open Source Robotics Foundation, Inc.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

#include <chrono>
#include <memory>

#include "rclcpp/rclcpp.hpp"
#include "std_msgs/msg/string.hpp"

using namespace std::chrono_literals;

/* This example creates a subclass of Node and uses std::bind() to register a
* member function as a callback from the timer. */

class MinimalPublisher : public rclcpp::Node
{
public:
MinimalPublisher()
: Node("minimal_publisher"), count_(0)
{
publisher_ = this->create_publisher<std_msgs::msg::String>("topic", 10);
timer_ = this->create_wall_timer(
500ms, std::bind(&MinimalPublisher::timer_callback, this));
}

private:
void timer_callback()
{
auto message = std_msgs::msg::String();
message.data = "Hello, world! " + std::to_string(count_++);
RCLCPP_INFO(this->get_logger(), "Publishing: '%s'", message.data.c_str());
publisher_->publish(message);
}
rclcpp::TimerBase::SharedPtr timer_;
rclcpp::Publisher<std_msgs::msg::String>::SharedPtr publisher_;
size_t count_;
};

int main(int argc, char *argv[])
{
rclcpp::init(argc, argv);
rclcpp::spin(std::make_shared<MinimalPublisher>());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wont go into too much depth because the ROS tutorials are pretty good, but I do want to mention a bit about spin.

The spin function is basically this:

while(true){
if there are any callbacks waiting in line/queue, process them in order they arrived.
}

The important things from that example being that:

  • Spin is blocking (i.e. once you call this line, it wont continue until ROS dies / you kill the program)
  • Callbacks are NOT processed in parallel. So if you have a callback that waits a really long time, all the other callbacks are stuck in line and that data wont get processed. In general, callbacks should be short and we want to grab the incoming data and move it somewhere else for processing so we can be free to handle more callbacks.

Copy link
Collaborator

Choose a reason for hiding this comment

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

In this case, we arent subscribing to any ROS topics, but the time processes callback functions in the same way.

rclcpp::shutdown();
return 0;
}
45 changes: 45 additions & 0 deletions 10_Examples/jett-onboarding/src/subscriber_member_function.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
// Copyright 2016 Open Source Robotics Foundation, Inc.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

#include <memory>

#include <rclcpp/rclcpp.hpp>
#include <std_msgs/msg/string.hpp>
using std::placeholders::_1;

class MinimalSubscriber : public rclcpp::Node
{
public:
MinimalSubscriber()
: Node("minimal_subscriber")
{
subscription_ = this->create_subscription<std_msgs::msg::String>(
"topic", 10, std::bind(&MinimalSubscriber::topic_callback, this, _1));
}

private:
void topic_callback(const std_msgs::msg::String::SharedPtr msg) const
{
RCLCPP_INFO(this->get_logger(), "I heard: '%s'", msg->data.c_str());
}
rclcpp::Subscription<std_msgs::msg::String>::SharedPtr subscription_;
};

int main(int argc, char *argv[])
{
rclcpp::init(argc, argv);
rclcpp::spin(std::make_shared<MinimalSubscriber>());
rclcpp::shutdown();
return 0;
}
36 changes: 36 additions & 0 deletions 10_Examples/jett-onboarding/src/talking.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
#include "talking.h"
#include <iostream>
#include "string.h"

using namespace std;

talking::talking()
{
this->num1 = 10;
}

talking::~talking()
{
}

string talking::talk(string words)
{
cout << words << endl;
return words;
}

int talking::numberPrint()
{
cout << num1 << endl;
return num1;
}

int talking::twoToPower(int i)
{
int value = 1;
while (i > 0) {
value *= 2;
i--;
}
return value;
}
23 changes: 23 additions & 0 deletions 10_Examples/jett-onboarding/src/talking.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
#ifndef TALKING_H_
#define TALKING_H_

#include <iostream>
#include "string.h"

using namespace std;
Copy link
Collaborator

Choose a reason for hiding this comment

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

One important rule for C/C++ programmers...
You should never put a using declaration in a header file. The reason is, we expect other people to include our header files, so any using statements will propogate to anything that includes it. That means everyone who uses your code is now unwillingly using everything in std. That is very problematic if they have their own version of string for example (or anything under std::)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Along these same lines, any code you see in our repo is almost always defined within a namespace so that its totally clear what we are referencing.

So in this case, we would probably put most of the code in this file in between the following two lines:

namespace jett-onboarding{

...

// All your code
...
} // namespace jett-onboarding


class talking
Copy link
Collaborator

Choose a reason for hiding this comment

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

The most popular convention for naming variables, classes, etc, is gonna be Google Style Guide:.

Its a good read, but im gonna cover main points here for C/C++:

  • classes are typically "PascalCase".
  • functions/methods are typically "camelCase"
  • variables are "snake_case"
  • members variables of a class should have an underscore at the end (or sometimes a ",_" prefix.

None of these rules will break your code or change functionality, but its very standard and so helps people understand what they are looking at. when they use your code

{
public:
int num1;

talking(); // constructor
~talking(); // destructor

string talk(string words);
Copy link
Collaborator

Choose a reason for hiding this comment

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

In general, we can split any code project into source files (.cpp), header files (.hpp), and tests. The header file is usually where you put all the nice comments explaining what functions do, what assumptions they make, if they throw errors, etc.

Well written header files and tests are often all the documentation you might get.

int numberPrint();

int twoToPower(int i);
};

#endif
43 changes: 43 additions & 0 deletions 10_Examples/jett-onboarding/src/testFile.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
#include "gtest/gtest.h"
#include "talking.h"
#include "string.h"

class TestExample : public ::testing::Test
{
protected:
TestExample()
{
talking_ = std::make_shared<talking>();
}

void SetUp() override
{
}

void TearDown() override
{
}

std::shared_ptr<talking> talking_;

};

TEST_F(TestExample, testTalk) {
//testing the talk function
EXPECT_EQ("Test", talking_->talk("Test"));
}

TEST_F(TestExample, testNumberPrint) {
EXPECT_TRUE(talking_->numberPrint() == talking_->num1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

A few more facts on gtest.

You can have many different EXPECT_XXX statements in a single test (and many times you will). In the case where you want a test to completely give up if something fails, gtest has ASSERT_XXX(...). If youd like it to continue running the tests, you can use EXPECT_XXX. I usually default to EXPECT.

}

TEST_F(TestExample, testTwoToPower) {
EXPECT_EQ(talking_->twoToPower(5) == (2 ^ 5));
Copy link
Collaborator

Choose a reason for hiding this comment

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

EXPECT_EQ takes two arguments, so thinking this will fail. You could switch to EXPECT_TRUE, or remove equality and make it comma separated

Copy link
Collaborator

Choose a reason for hiding this comment

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

Another important thing to know:
EXPECT_EQ is fine for ints and bools, but floats and doubles can be fussy. Sometimes, they are basically the same number, but very very very slightly off due to machine precision. So for example, your equation might spit out 3.0000000000001 instead of 3.

For those, gtest has EXPECT_FLOAT_EQ, EXPECT_DOUBLE_EQ, and EXPECT_NEAR.

}


// int main(int argc, char ** argv)
Copy link
Collaborator

Choose a reason for hiding this comment

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

you can either include this main and link against gtest in your cmake, or you can leave the main out and link against gtest_main in cmake for short. The latter is fine unless you need to do any special setup before running tests.

// {
// ::testing::InitGoogleTest(&argc, argv);
// return RUN_ALL_TESTS();
// }
Loading