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

Feature/cpp overhaul #7

Open
wants to merge 3 commits into
base: dev
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
55 changes: 55 additions & 0 deletions bebot/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
## [main]

# See: https://cliutils.gitlab.io/modern-cmake/chapters/basics/example.html

# Almost all CMake files should start with this
# You should always specify a range with the newest
# and oldest tested versions of CMake. This will ensure
# you pick up the best policies.
cmake_minimum_required(VERSION 3.0.0)

Copy link
Collaborator

Choose a reason for hiding this comment

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

You probably want to specify C++ language standard and compiler flags

# This is your project statement. You should always list languages;
# Listing the version is nice here since it sets lots of useful variables
project(
bebot
VERSION 0.1.0
LANGUAGES CXX
DESCRIPTION "BeBOT Library.")

add_subdirectory(src)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wouldn't add_subdirectory here. I would expect you to really only have one or two build targets, a library to link to and maybe an executable, so it's better to just add_library(bebot src/...) here, etc rather than make a mostly redundant CMakeList in src to split across.

add_subdirectory(apps)

# # If you set any CMAKE_ variables, that can go here.
# # (But usually don't do this, except maybe for C++ standard)

# # Find packages go here.

# # You should usually split this into folders, but this is a simple example

# This is a "default" library, and will match the *** variable setting.
# Other common choices are STATIC, SHARED, and MODULE
# Including header files here helps IDEs but is not required.
# Output libname matches target name, with the usual extensions on your system
# add_library(bebot SHARED
# src/bebot.cpp
# include/bebot/bebot.hpp)

# # Link each target with other targets or add options, etc.

# #include_directories("${CMAKE_CURRENT_SOURCE_DIR}/include")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh yeah, just uncomment this and get rid of bebot subdirectory within include


# add_executable(MyApp apps/main.cpp)

# target_include_directories(bebot PRIVATE include)
# target_include_directories(bebot PRIVATE src)

# # set(CPACK_PROJECT_NAME ${PROJECT_NAME})
# # set(CPACK_PROJECT_VERSION ${PROJECT_VERSION})
# # include(CPack)

# # # Adding something we can run - Output name matches target name
# # add_executable(MyExample simple_example.cpp)

# # # Make sure you link your targets with this command. It can also link libraries and
# # # even flags, so linking a target that does not exist will not give a configure-time error.
# # target_link_libraries(MyExample PRIVATE MyLibExample)
2 changes: 2 additions & 0 deletions bebot/apps/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
add_executable(main main.cpp)
target_link_libraries(main PRIVATE bebot)
25 changes: 25 additions & 0 deletions bebot/apps/main.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
#include <bebot/bebot.hpp>
#include <Eigen/Dense>
#include <iostream>

using namespace std;

int main() {
Eigen::MatrixXd cpts{
{0, 1, 2, 3, 4, 5},
{3, 6, 2, 9, 6, 8},
{3, 4, 6, 4, 7, 9}
};

bebot::BeBOT c(cpts);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where possible, prefer {} initialization. It's safer and avoids the most vexing parse.


cout << cpts << endl << "---" << endl;
cout << c.get_cpts() << endl << "---" << endl;

Eigen::MatrixXd new_cpts{
{0, 1, 2},
{3, 4, 2}
};
c.set_cpts(new_cpts);
cout << c.get_cpts() << endl;
}
29 changes: 29 additions & 0 deletions bebot/include/bebot/bebot.hpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
#pragma once

#include <Eigen/Dense>

namespace bebot {
class BeBOT {
Copy link
Collaborator

Choose a reason for hiding this comment

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

BeBOT is the name of the library but you probably don't want to name this class that. Maybe something like bebot::ControlProblem (this class describes a control problem to solve) or bebot::TrajectorySolver (this class solves for your trajectory), etc


private:
double _t0, _tf;
Eigen::MatrixXd _cpts;

public:
BeBOT() = default;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Default constructor doesn't instantiate cpts, t0 and tf -> undefined behaviour bonanza. Make one constructor BeBOT(const Eigen::Ref<Eigen::MatrixXd>& cpts, const double t0=0, const double tf=1);

~BeBOT() = default;

BeBOT(const Eigen::Ref<Eigen::MatrixXd>& cpts);
BeBOT(const Eigen::Ref<Eigen::MatrixXd>& cpts, const double tf);
BeBOT(const Eigen::Ref<Eigen::MatrixXd>& cpts, const double t0, const double tf);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand the purpose of multiple constructors. What happens if initial and final time are not specified? Scaled to 0..1? In that case, give the parameters default constructors.


void set_cpts(const Eigen::Ref<Eigen::MatrixXd>& cpts);
Eigen::MatrixXd get_cpts();
Copy link
Collaborator

Choose a reason for hiding this comment

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

const& return will likely be faster when there's more control points


void set_t0(const double t0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might be a philosophy thing but unless there's a reason you expect there to be more complexity in assignment/reading, getters/setters like this are antipatterns. Even when they're not, instead of .get_t0(), I would prefer .t0(). On that note, .initial_time and final_time?

double get_t0();

void set_tf(const double tf);
double get_tf();
};
}
5 changes: 5 additions & 0 deletions bebot/src/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
add_library(bebot bebot.cpp ../include/bebot/bebot.hpp)
Copy link
Collaborator

Choose a reason for hiding this comment

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

split CMake functoin calls over multiple files.

add_library(bebot 
  bebot.cpp
  othercpp.cpp
  etc.cpp
)

Headers shouldn't be part of your add_library call, it should be part of an include_directories() call

Copy link
Collaborator

Choose a reason for hiding this comment

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

On that note, you don't need a include/bebot directory. There's nothing from another project here to expose to include, so just make it include and put only the headers you want publically includeable by other projects there, and keep your other internally available hpp files in your src directory.

target_include_directories(bebot PUBLIC ../include)

find_package (Eigen3 3.3 REQUIRED NO_MODULE)
target_link_libraries(bebot Eigen3::Eigen)
47 changes: 47 additions & 0 deletions bebot/src/bebot.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
#include <bebot/bebot.hpp>
#include <Eigen/Dense>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Eigen is included in the header already

#include <iostream>
Copy link
Collaborator

Choose a reason for hiding this comment

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

no IO calls in this file, and there probably shouldn't be direct IO calls. Logging functions if necessary.


using namespace bebot;

BeBOT::BeBOT(const Eigen::Ref<Eigen::MatrixXd>& cpts) {
set_cpts(cpts);
set_t0(0.0);
set_tf(1.0);
}

BeBOT::BeBOT(const Eigen::Ref<Eigen::MatrixXd>& cpts, const double tf) {
set_cpts(cpts);
set_t0(0.0);
set_tf(tf);
}

BeBOT::BeBOT(const Eigen::Ref<Eigen::MatrixXd>& cpts, const double t0, const double tf) {
set_cpts(cpts);
set_t0(t0);
set_tf(tf);
}

void BeBOT::set_cpts(const Eigen::Ref<Eigen::MatrixXd>& cpts) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

More descriptive naming is good. set_control_points isn't that many more characters. In general, we try to avoid writing C++ like it was C, and the horrible idmtcNmng_cnvns that exist in C shouldnt happen in C++

_cpts = cpts;
}

Eigen::MatrixXd BeBOT::get_cpts() {
return _cpts;
}

void BeBOT::set_t0(const double t0) {
_t0 = t0;
}

double BeBOT::get_t0() {
return _t0;
}

void BeBOT::set_tf(const double tf) {
_tf = tf;
}

double BeBOT::get_tf() {
return _tf;
}