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

Dynamic Traction Demonstration State Machine #123

Open
wants to merge 24 commits into
base: master
Choose a base branch
from
Open

Conversation

SnickeyX
Copy link
Contributor

@SnickeyX SnickeyX commented Mar 9, 2022

Description

Implementing demo_state_machine. Added missing instances of kPreReady in main.cpp under Brakes and fake_trajectory.cpp under Sensors.

  • Adding gtests
  • Acceleration Time
  • Braking Command
  • Add UI

@SnickeyX SnickeyX changed the title demo-sandbox STM-demo-sandbox Mar 9, 2022
@SnickeyX SnickeyX self-assigned this Mar 9, 2022
@codecov-commenter
Copy link

codecov-commenter commented Mar 9, 2022

Codecov Report

Merging #123 (d4cd484) into master (516a539) will increase coverage by 2.58%.
The diff coverage is 94.70%.

@@            Coverage Diff             @@
##           master     #123      +/-   ##
==========================================
+ Coverage   38.20%   40.78%   +2.58%     
==========================================
  Files          69       73       +4     
  Lines        3942     4129     +187     
==========================================
+ Hits         1506     1684     +178     
- Misses       2436     2445       +9     
Impacted Files Coverage Δ
src/data/data.hpp 100.00% <ø> (ø)
src/propulsion/fake_controller.cpp 0.00% <0.00%> (ø)
src/state_machine/state.cpp 97.87% <ø> (ø)
src/utils/system.cpp 55.30% <56.25%> (+0.05%) ⬆️
src/demo_state_machine/state.cpp 97.84% <97.84%> (ø)
src/demo_state_machine/main.cpp 100.00% <100.00%> (ø)
src/demo_state_machine/state.hpp 100.00% <100.00%> (ø)
src/demo_state_machine/transitions.cpp 100.00% <100.00%> (ø)
src/utils/io/gpio.cpp 28.65% <100.00%> (ø)
src/utils/timer.cpp 26.66% <100.00%> (+5.23%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 516a539...d4cd484. Read the comment docs.

@SnickeyX SnickeyX requested a review from mifrandir March 10, 2022 12:54
@SnickeyX
Copy link
Contributor Author

SnickeyX commented Mar 10, 2022

@MiltFra its not complete yet, just wanted to check if what I've done so far is okay

@mifrandir mifrandir changed the title STM-demo-sandbox Dynamic Traction Demonstration State Machine Mar 12, 2022
Copy link
Member

@mifrandir mifrandir left a comment

Choose a reason for hiding this comment

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

Looking good so far.

src/demo_state_machine/transitions.cpp Outdated Show resolved Hide resolved
src/demo_state_machine/transitions.hpp Show resolved Hide resolved
@SnickeyX
Copy link
Contributor Author

demo UI does not work atm, still working on it

@SnickeyX SnickeyX requested review from kshxtij and mifrandir May 5, 2022 13:21
@mifrandir
Copy link
Member

@kshxtij please ping me once you have thoroughly reviewed this.


#include <chrono>

namespace hyped {
Copy link
Member

Choose a reason for hiding this comment

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

namespace hyped::demo_state_machine ?


const uint64_t time_now = utils::Timer::getTimeMicros();
const int64_t time_elapsed = time_now - instance_.acceleration_start_;

Copy link
Member

Choose a reason for hiding this comment

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

If these are both timestamps, why are they different types?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

getTimeMacros() gives a uint64_t so I have to use it, but for time_elapsed we need to worry about it being signed (positive specifically) coz negative time difference should not happen and we should know that's the case

// Navigation Data Events
//--------------------------------------------------------------------------------------

bool checkEnteredBrakingZone(utils::Logger &log, const data::Navigation &nav_data)
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this in the demo state machine?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope, will remove


bool checkPodStopped(utils::Logger &log, const data::Navigation &nav_data)
{
if (nav_data.velocity > 0) return false;
Copy link
Member

Choose a reason for hiding this comment

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

I'm not 100% sure it will be exactly zero since it is type float. Maybe we could use a small number as a tolerance?


namespace hyped {

namespace demo_state_machine {
Copy link
Member

Choose a reason for hiding this comment

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

namespace hyped::demo_state_machine?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

syntactic difference, will change it still

return true;
}

bool checkReachedMaxVelocity(utils::Logger &log, const data::Navigation &nav_data)
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this? I thought the pod wasn't really moving in demo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope, will remove

@@ -123,7 +123,7 @@ void Gpio::uninitialise()
void Gpio::exportGPIO()
{
if (!initialised_) {
log_.error("servise has not been initialised");
log_.error("service has not been initialised");
Copy link
Member

Choose a reason for hiding this comment

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

😍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants