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

Fix type error in state processor #48

Closed
wants to merge 2 commits into from

Conversation

TomLonergan03
Copy link
Contributor

@TomLonergan03 TomLonergan03 commented Oct 18, 2021

Description

Fixes type error in state_processor.cpp from #35

Changes

  • converted int32_t to float

@codecov-commenter
Copy link

codecov-commenter commented Oct 18, 2021

Codecov Report

Merging #48 (32b80bb) into master (c8b148e) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #48   +/-   ##
=======================================
  Coverage   29.99%   29.99%           
=======================================
  Files          72       72           
  Lines        3797     3797           
=======================================
  Hits         1139     1139           
  Misses       2658     2658           
Impacted Files Coverage Δ
src/propulsion/RPM_regulator.cpp 0.00% <ø> (ø)
src/propulsion/RPM_regulator.hpp 0.00% <ø> (ø)

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 c8b148e...32b80bb. Read the comment docs.

@@ -10,8 +10,8 @@ RPM_Regulator::RPM_Regulator(Logger &log) : log_(log), current_index(0), failure
{
}

int32_t RPM_Regulator::calculateRPM(int32_t act_velocity, int32_t act_rpm, int32_t act_current,
int32_t act_temp)
float RPM_Regulator::calculateRPM(float act_velocity, int32_t act_rpm, int32_t act_current,
Copy link
Member

Choose a reason for hiding this comment

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

This is not quite where the problem is. We use std::round to obtain an integer, so the return type of int32_t makes sense! However, we are passing in an int32_t which is problematic because it should be a floating-point number (as you have correctly identified). However, all our floating-point numbers are of type data::nav_t (which is just an alias for double) so you should replace float with this. Further, I'd say you should fix this issue in #44.

@@ -124,7 +124,7 @@ void StateProcessor::accelerate()
int32_t act_current = calcMaxCurrent();
int32_t act_temp = calcMaxTemp(controllers);

int32_t rpm = regulator.calculateRPM(velocity, act_rpm, act_current, act_temp);
float rpm = regulator.calculateRPM(velocity, act_rpm, act_current, act_temp);
Copy link
Member

Choose a reason for hiding this comment

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

The issue here is not the return type. RPM is a quantity that is being measured in integer values. However, the member field velocity (which should be called velocity_ but actually has no right to be a member field in the first place and should instead be replaced with a local variable) is of type float. The velocity we get from Navigation is of type data::nav_t so we should stay consistent with that.

@TomLonergan03 TomLonergan03 deleted the mtc-state_processor-typeerror branch October 19, 2021 00:57
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.

3 participants