hyp-ed / hyped-2022 Goto Github PK

GitHub doesn't allow us to open PRs and issues on the wiki. One way to avoid this seems to be to create a mirror repository with a GitHub action that allows us to push to the wiki (and maybe do dynamic re-linking) upon new commits to master. This may be possible through GitHub actions.

All files need checked for basic issues

Files assigned:

• state_processor.cpp @maryk-123
• state_processor.hpp @maryk-123
• file_reader.cpp @TomLonergan03
• file_reader.hpp @TomLonergan03
• RPM_regulator.hpp @TomLonergan03
• RPM_regulator.cpp @TomLonergan03
• controller.cpp @TomLonergan03
• controller.hpp @TomLonergan03
• controller_interface.hpp @TomLonergan03
• main.cpp @RoxieBethyl
• main.hpp @RoxieBethyl
• deal with issue #35 @TomLonergan03
• fake_controller.cpp @TomLonergan03
• fake_controller.hpp @TomLonergan03

Unassigned:

Graphs on the left of the GUI need to be manually selected every time it runs. Graphs should appear by default instead.

Currently, every file has a comment at the top which gives general information like a description, the author and the licensing. While this is intended to be useful, it turns out that there is no point in keeping this.

• To find the last edit date and the authors, git blame or GitHub are far more useful and accurate and don't require any human input.
• The descriptions tend to be out of date or empty. Additionally, I would argue that such general documentation should be in the wiki.
• The license can be put at the top level of the directory. Since any code is copyrighted by default, and licenses only weaken those restrictions, there is no requirement whatsoever to make "absolutely doubly sure" that whoever may read the code is aware of the licensing. If they want to use it, they have to be aware of the license.

Will double check T-Sensor datasheet. Then I will make a PR with any necessary changes

Fix basic issues in the following files. (Naming, typing, adding const, using namespace etc.)

• client.[ch]pp - @rachel275
• main.[ch]pp and sendloop.[ch]pp - @sashank17
• recvloop.[ch]pp - @dhb03
• writer.[ch]pp - @adenhaus

When the GUI is run with no fake system flags set, terminal output iOS displayed on the front end. However, when we run it with the fake system flags, there is no terminal output on the front end unless it errors out.

At the moment, the pod-side code makes heavy use of threading. This makes sense because we have different components that are quite independent. However, the BBB only has a single core so we cannot make use of any parallelism whatsoever.

Further, every module has a main loop that reruns the same logic over and over again. This means it is easy break up the code into chunks that can be run consecutively without blocking each other for too long. This should lead to a speedup because we don't need to rely on the OS to switch between the threads. Further, it will allow us to use callbacks. For example, some parts of the code currently use sleeps to account for mechanical actions, like engaging brakes. Consider the following fictional code:

engageBrakes();
Thread::sleep(1000);
if (!checkBrakesEngaged()) {
  criticalFaillure("Could not engage brakes");
}

This could be improved by using a central event queue as follows:

engageBrakes()
Core::schedule(1000, [&]{
  if (checkBrakesEngaged()) {
    criticalFailure("Could not engage brakes");
  }
});

This would have several advantages:

1. Removes race conditions and make the program more deterministic.
2. Allows for simple scheduling.
3. Gives finer control in terms of priorities.
4. Reduces thread overhead and scheduling costs.
5. Allows us to get rid of singleton instances, e.g. for data and system.
6. Allows to use (const) references and modifying/reading from those directly so we save on locking.
7. Simplifies testing as we can verify properties of data after each iteration.
8. Allows us to (eventually) get rid of check based code e.g. in State Machine. Instead, we could use updates directly as everything would be in the same thread so transitions could happen instantly after a change occurs.

We want to be able to

1. Compile for Linux x64 on Linux x64.
2. Compile for ARM v7 (BBB) on Linux x64.
3. Compile for ARM v7 on ARM v7.

#84 cannot be merged until mission-control-2022 works with the new json structure. Currently, nothing on the GUI shows when sent the new json and it seems like not all the json is being written to file by the server. Not sure if that's because it's not all being sent, or if there is a receiving/writing issue.

Assigned Files:

• temperature.cpp - @supermegho
• temperature.hpp - @supermegho
• gpio_counter.cpp - @AlbinGrip99
• gpio_counter.hpp - @AlbinGrip99
• gpio_manager.cpp - @kshxtij
• gpio_manager.hpp - @kshxtij
• imu.cpp - @AlbinGrip99
• imu.hpp - @AlbinGrip99
• bms.cpp - Anant
• bms.hpp - Anant
• bms_manager.cpp - @Davidhqm
• bms_manager.hpp - @Davidhqm

Unassigned Files:

• imu_manager.cpp
• imu_manager.hpp
• interface.hpp
• main.cpp
• main.hpp
• fake_batteries.cpp
• fake_batteries.hpp
• fake_gpio_counter.cpp
• fake_gpio_counter.hpp
• fake_gpio_counter.hpp
• fake_imu.cpp
• fake_imu.hpp
• fake_temperature.cpp
• fake_temperature.hpp

Using a different build system has been discussed before. There are many reasons why we should use CMake instead of pure GNU Make:

1. It's the industry standard and thus a tool worth getting used to.
2. It's more transparent. Since it takes the builds to a higher level, it's easier to modify and manage.
3. IDEs are able to load CMake projects and improve their support based on that.

We should also consider upgrading to C++20 and using Clang.

We are still using many raw pointers in places where they are not required. Raw pointers are difficult to analyse and should be avoided unless strictly necessary. If no pointer arithmetic is required and the reference itself does not have to be stored in a collection, references are sufficient.

Further, we should be using the const keyword as much as possible. While it does not make the code prettier, it does provide helpful compile-time guarantees which may allow us to avoid some bugs. Further, it gives the developers good insight into how certain values are used.

Ideally, we would want to have a line like

static Logger log("STM", STM_LL);

Then we could remove the passing of loggers entirely. This would also allow us to set the log levels through CMake flags like so:

$ cmake .. -DSTM_LL=4

This has already been started here:

In RPM_Regulator::calculateRPM when the temperature is over the maximum temperature the output is to maintain current RPMs. This feels counterintuitive and needs confirmed whether it is correct behaviour.

Please refer to this comment here:

Here we are converting a data::nav_t to int32_t which loses precision unintentionally.

In gpio_manager.cpp, the default case could be triggered once the pod is in kCalibrating - but this would cause unnecessary critical failure. Is this intentional?

There are some // NOLINT and // TODO comments in the source code. These should be removed.

In case of the TODOs we should decide whether or not they are still relevant and if that's the case create a corresponding issue.

At the moment we require Sims to provide us with files containing sensor readings to test our software. This is problematic because once we change the pod's behaviour, e.g. by adding a state to the state machine, the sims data is off and we cannot simulate a run. This could be fixed by defining a ground truth displacement/velocity/acceleration curve that can then be used to generate sensor readings. This is close to what Sims are doing anyways so we can just rewrite their MATLAB code into a C++ component.

This could even incorporate motor controls as we could make the acceleration dependent on the motors and what they are currently doing.

In any case, Software should not rely on one particular pod design.

Cf. https://github.com/Hyp-ed/hyped-2022/wiki/Production-Tests

The GUI needs to be made to work with the values in the fake_config file when run with fake system.

The current controller implementation looks like it should work but I can't find any good documentation that explains the reasoning behind it. Also, some refactors are required.

We need to make sure that this is modernised and documented.

https://github.com/Hyp-ed/hyped-2022/blob/master/src/propulsion/controller.cpp

We are referring to the brakes as embrakes even though they are not just emergency brakes anymore. That should change.

For example the GPIO counter class, which inherits from thread and is run as such, provides a method getData which is used in other threads but doesn't use a lock.

New states which have recently been added aren't displayed bin the state section of the GUI. Support for these needs to be added on the front end.

For example the current version

State *Idle::checkTransition(Logger &log)
{
  updateModuleData();

  bool emergency = checkEmergency(log, embrakes_data_, nav_data_, batteries_data_, telemetry_data_,
                                  sensors_data_, motors_data_);
  if (emergency) { return FailureStopped::getInstance(); }

  bool calibrate_command = checkCalibrateCommand(log, telemetry_data_);
  if (!calibrate_command) { return nullptr; }

  bool all_initialised = checkModulesInitialised(log, embrakes_data_, nav_data_, batteries_data_,
                                                 telemetry_data_, sensors_data_, motors_data_);
  if (all_initialised) { return Calibrating::getInstance(); }

  return nullptr;
}

should be rewritten to

const State *Idle::checkTransition(Logger &log)
{
  updateModuleData();

  const bool emergency = checkEmergency(log, embrakes_data_, nav_data_, batteries_data_, telemetry_data_,
                                  sensors_data_, motors_data_);
  if (emergency) { return FailureStopped::getInstance(); }

  const bool calibrate_command = checkCalibrateCommand(log, telemetry_data_);
  if (!calibrate_command) { return nullptr; }

  const bool all_initialised = checkModulesInitialised(log, embrakes_data_, nav_data_, batteries_data_,
                                                 telemetry_data_, sensors_data_, motors_data_);
  if (all_initialised) { return Calibrating::getInstance(); }

  return nullptr;
}

Alternatives like

std::optional<std::unique_ptr<State>> Idle::checkTransition(Logger &log)

also seem possible.
Similarly, all transition functions should only use const references to the data objects.

Ideally, we would want to

1. Have proper configurations for all subsystems.
2. Have a standard config file format like XML/XSD, JSON, TOML, YAML, ...
3. A config parsing implementation that uses a library (e.g. [Boost.Program](https://www.boost.org/doc/libs/1_76_0/doc/html/program_options.

cf. this line

Assigned files for basic clean-up:

• main.cpp and main.hpp @SnickeyX
• state.cpp and state.hpp @amadrafi
• transitions.cpp and transitions.hpp @SnickeyX

Cleaning up easy issues in Java and JavaScript files in mission-control-2022 repo. If you're using IntelliJ, it will suggest a couple of improvements for the Java files, for example. Scan through the files and if you see some obvious things you can fix, fix them, else leave it. No need to commit for the sake of committing.

Issues for mission-control-2022 will be opened here, but open your PR on the repo itself.

Java files in question are in mission-control-2022/src/main/java/server.

• Controller.java, Server.java - @adenhaus

JavaScript files are in mission-control-2022/frontend/src.

• App.js, ConfigManager.js, DataTools.js, index.js, serviceWorker.js - @sashank17

In the screenshot of the GUI in the FDD there is a panel displaying battery info on the GUI. When we run it, this panel isn't visible.

All of our software is designed to handle a full pod run. This obviously means that we don't have direct access to all the features the pod has to offer through manual controls for safety reasons. However, we will get into testing soon and we need to have a binary that allows us to do everything that we would want in such a setting.

hyped-2022/src/utils/math/integrator.hpp

Refactor and clean up utils/math

• differentiator.hpp @weijuinlee
• integrator.hpp @weijuinlee
• kalman_multivariate.hpp @flori-margaritescu @Akshath29
• kalman_multivariate.cpp @flori-margaritescu @Akshath29
• quaternion.hpp @weijuinlee
• vector.hpp @Akshath29

https://github.com/Hyp-ed/hyped-2022/blob/master/src/propulsion/file_reader.cpp, see the comment.

While rewriting, we should remove the C-style code.

• reference to a vector instead of a raw pointer and a length
• read file through C++ standard library tools, don't use C style functions