-
Notifications
You must be signed in to change notification settings - Fork 1
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
base: master
Are you sure you want to change the base?
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
private: | ||
utils::Logger &log_; | ||
utils::io::can::Frame nucleo_message_; | ||
CanSender sender_; |
There was a problem hiding this comment.
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.
void CanReceiver::processNewData(utils::io::can::Frame &message) | ||
{ | ||
uint32_t id = message.id; | ||
if (id == kEmgyTransmit + node_id_) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
void CanReceiver::processNewData(utils::io::can::Frame &message) | ||
{ | ||
uint32_t id = message.id; | ||
if (id == kEmgyTransmit + node_id_) { |
There was a problem hiding this comment.
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?
|
||
void processNewData(utils::io::can::Frame &message) override; | ||
|
||
bool hasId(const uint32_t id, bool extended) override; |
There was a problem hiding this comment.
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?
can_frame_.len = 4; | ||
} | ||
|
||
void NucleoManager::sendNucleoFrequency(uint32_t target_frequency) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aaaaand once more...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const
😂
There was a problem hiding this comment.
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
There was a problem hiding this 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?
is_sending_ = false; | ||
} | ||
|
||
bool FakeCanReceiver::hasId(const uint32_t, bool) |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const?
Description
Implements CAN connection to STM board