Jett Onboarding New

[jettVanbrocklin]

Description

Fixed test file and CMAKE

Reviewers

• Required: @MaxxWilson

---

Changelog

• Test File changed to follow 'gtest' convention

Testing

Automatic

Each Talking Class Function is Called

• talk(string) checked to execute and return string.
• numberPrint() checked to return the class variable num1.
• twoToPower(int) checked to return the proper integer value.

Checklist

• Confirmed all tests pass on a clean build
• Added reviewers in Github
• Posted PR Summary to Discord PR's Channel
• Ran uncrustify on any modified C++ files
• Ran Colcon Lint for any modified CMakeLists.txt or Package.xml

[MaxxWilson]

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).

[MaxxWilson]

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.

[MaxxWilson]

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.

[MaxxWilson]

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).

[MaxxWilson]

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.

[MaxxWilson]

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).

[MaxxWilson]

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.

[MaxxWilson]

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

[MaxxWilson]

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.

[MaxxWilson]

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

[MaxxWilson]

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.

[MaxxWilson]

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.

[MaxxWilson]

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::)

[MaxxWilson]

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

[MaxxWilson]

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

[MaxxWilson]

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.

[MaxxWilson]

Overall, looks super great!!

I try to be very pedantic to help people learn new stuff, so left a lot of comments. This is onboarding, so I dont expect you to go change any of that, we can call this onboarding concluded. Other times though, this is the point where people would request changes, or ask for clarification.

One last note.
We dont typically merge onboarding branches into develop (because it clutters). We have a separate branch called "onboarding' if i recall. If you want to feel the satisfaction of pressing merge button, retarget this PR on to onboarding branch. If you dont care, then we can move on to robot tasks.

Thanks for sticking with onboarding process! If you have any questions/concerns/suggestions about onboarding overall, I'd love to hear them.