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

Mtc Nucleo board connection #115

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

Mtc Nucleo board connection #115

wants to merge 31 commits into from

Conversation

TomLonergan03
Copy link
Contributor

Description

Implements CAN connection to STM board

@TomLonergan03 TomLonergan03 self-assigned this Feb 12, 2022
@codecov-commenter
Copy link

codecov-commenter commented Feb 12, 2022

Codecov Report

Merging #115 (b7948a3) into master (b4144b8) will increase coverage by 0.08%.
The diff coverage is 15.15%.

@@            Coverage Diff             @@
##           master     #115      +/-   ##
==========================================
+ Coverage   34.01%   34.10%   +0.08%     
==========================================
  Files          67       69       +2     
  Lines        3545     3545              
==========================================
+ Hits         1206     1209       +3     
+ Misses       2339     2336       -3     
Impacted Files Coverage Δ
src/propulsion/can/can_sender.cpp 33.33% <0.00%> (+25.92%) ⬆️
src/propulsion/controller.cpp 1.46% <0.00%> (ø)
src/propulsion/fake_controller.cpp 0.00% <ø> (ø)
src/propulsion/fake_controller.hpp 0.00% <ø> (ø)
src/propulsion/main.cpp 21.81% <ø> (ø)
src/propulsion/rpm_regulator.cpp 0.00% <0.00%> (ø)
src/utils/io/spi.cpp 0.00% <ø> (ø)
src/propulsion/can/can_receiver.cpp 6.66% <6.66%> (ø)
src/propulsion/state_processor.cpp 6.09% <14.28%> (-0.16%) ⬇️
src/propulsion/nucleo_manager.cpp 42.85% <42.85%> (ø)

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 b4144b8...b7948a3. Read the comment docs.

@mifrandir mifrandir changed the title Mtc STM connection Mtc STN connection Feb 14, 2022
src/propulsion/can/can_sender.hpp Show resolved Hide resolved
src/propulsion/can/can_transceiver.hpp Outdated Show resolved Hide resolved
src/propulsion/nucleo_manager.cpp Outdated Show resolved Hide resolved
src/propulsion/nucleo_manager.hpp Outdated Show resolved Hide resolved
src/propulsion/nucleo_manager.hpp Outdated Show resolved Hide resolved
private:
utils::Logger &log_;
utils::io::can::Frame nucleo_message_;
CanSender sender_;
Copy link
Member

Choose a reason for hiding this comment

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

I don't think the whole fake CAN setup is being used (or even works) at the moment, but to make this easier in the future I would make this into a ICanSender (rename the ISender interface for clarity). Then one could pass in the sender to be used by this class.

In fact this sounds like a suitable refactor. You would probably want to do this everywhere. (Controller, FakeController, IController, ...) Of course we would then still get the CAN instance, but only at a higher level.

src/propulsion/state_processor.cpp Outdated Show resolved Hide resolved
src/propulsion/state_processor.hpp Outdated Show resolved Hide resolved
@TomLonergan03 TomLonergan03 changed the title Mtc STN connection Mtc Nucleo board connection Mar 11, 2022
src/propulsion/can/can_sender.cpp Outdated Show resolved Hide resolved
src/propulsion/can/can_sender.cpp Outdated Show resolved Hide resolved
src/propulsion/nucleo_manager.cpp Outdated Show resolved Hide resolved
src/propulsion/state_processor.cpp Outdated Show resolved Hide resolved
src/propulsion/can/can_ids.hpp Outdated Show resolved Hide resolved
void CanReceiver::processNewData(utils::io::can::Frame &message)
{
uint32_t id = message.id;
if (id == kEmgyTransmit + node_id_) {
Copy link
Member

Choose a reason for hiding this comment

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

How do we ensure that there are no conflicts here? Do we limit node IDs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Each node_id_ is numbered from 0 and there is one node for each motor controller so we won't have one overflow unless we have 21 motor controllers

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 know the BMS is not using these nodes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BMS starts at node 300 as far as i can tell

src/propulsion/can/can_receiver.cpp Outdated Show resolved Hide resolved
src/propulsion/can/can_receiver.cpp Outdated Show resolved Hide resolved
src/propulsion/can/can_receiver.hpp Outdated Show resolved Hide resolved
src/propulsion/controller.hpp Outdated Show resolved Hide resolved
src/propulsion/nucleo_manager.cpp Outdated Show resolved Hide resolved
src/propulsion/rpm_regulator.hpp Outdated Show resolved Hide resolved
src/propulsion/state_processor.cpp Outdated Show resolved Hide resolved
src/propulsion/state_processor.hpp Show resolved Hide resolved
src/propulsion/state_processor.hpp Show resolved Hide resolved
void CanReceiver::processNewData(utils::io::can::Frame &message)
{
uint32_t id = message.id;
if (id == kEmgyTransmit + node_id_) {
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 know the BMS is not using these nodes?

src/propulsion/can/can_sender.hpp Outdated Show resolved Hide resolved

void processNewData(utils::io::can::Frame &message) override;

bool hasId(const uint32_t id, bool extended) override;
Copy link
Member

Choose a reason for hiding this comment

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

Can we add a comment here why the boolean isn't used?

src/propulsion/can/fake_can_sender.hpp Outdated Show resolved Hide resolved
can_frame_.len = 4;
}

void NucleoManager::sendNucleoFrequency(uint32_t target_frequency)
Copy link
Member

Choose a reason for hiding this comment

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

Aaaaand once more...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what?

Copy link
Member

Choose a reason for hiding this comment

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

const 😂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

target_frequency isn't const. gets bitshifted a couple lines later

src/propulsion/nucleo_manager.cpp Outdated Show resolved Hide resolved
src/propulsion/rpm_regulator.hpp Outdated Show resolved Hide resolved
src/propulsion/state_processor.cpp Outdated Show resolved Hide resolved
@maxguy2001 maxguy2001 self-requested a review March 24, 2022 16:45
Copy link
Member

@maxguy2001 maxguy2001 left a comment

Choose a reason for hiding this comment

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

A lot of 'uint8_t' types and couldn't find where they all come from so don't know how big the numbers are really getting sometimes but worried they might be bigger than 255?

src/propulsion/can/can_receiver.cpp Outdated Show resolved Hide resolved
src/propulsion/can/can_receiver.hpp Show resolved Hide resolved
src/propulsion/can/can_receiver.hpp Outdated Show resolved Hide resolved
src/propulsion/can/fake_can_receiver.cpp Outdated Show resolved Hide resolved
is_sending_ = false;
}

bool FakeCanReceiver::hasId(const uint32_t, bool)
Copy link
Member

Choose a reason for hiding this comment

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

not passing in any arguments?

{
can_frame_.id = kNucleoTransmit;
can_frame_.extended = false;
can_frame_.len = 4;
Copy link
Member

Choose a reason for hiding this comment

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

const?

src/propulsion/rpm_regulator.cpp Outdated Show resolved Hide resolved
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.

4 participants