CANModule to fit CAN into app restructure#407
Conversation
wmmc88
left a comment
wmmc88
left a comment
There was a problem hiding this comment.
some of the functions you modified were tied to mbed threads. u need to remove the start calls to those threads in CANInterface.cpp
| void CANInterface::txProcessor(void) { | ||
| while (true) { | ||
| auto startTime = Kernel::Clock::now(); | ||
| auto startTime = Kernel::Clock::now(); |
There was a problem hiding this comment.
startTime is now no longer used here
| @@ -55,12 +55,8 @@ void CANInterface::rxPostman(void) { | |||
| void CANInterface::rxClient(void) { | |||
| while (true) { | |||
There was a problem hiding this comment.
should no longer be while true if its a 1ms periodic function
| // Check if a message has arrived: | ||
| mail = m_rxMailbox.try_get(); | ||
|
|
||
| MBED_ASSERT(mail != nullptr); |
There was a problem hiding this comment.
I agree with the above logic change on lines 58 and 59, but now it results in try_get sometimes returning nullptr if the mailbox is empty. should change this assert to maybe a debug print and early return(or just an if block to block execution of rest of this function. personally, i prefer single return statements from functions in fw code)
There was a problem hiding this comment.
Would the MBED_ASSERT not account for that?
There was a problem hiding this comment.
MBED_ASSERT would actually break the program, I agree that we should remove it and just wrap the logic in an if block
| m_numCANTXFaults++; | ||
| } | ||
| MBED_ASSERT(m_txMailboxOneShot.free(mail) == osOK); | ||
| ThisThread::sleep_for(TX_INTERDELAY); |
There was a problem hiding this comment.
should revisit this and see if this sleep is actually necessary. there really should be no sleeps in the periodic functions cuz itll screw up all the timings
|
|
||
| m_numStreamedMsgsSent++; | ||
|
|
||
| ThisThread::sleep_for(TX_INTERDELAY); |
There was a problem hiding this comment.
ditto comment about sleeps in periodic
| void CANModule::periodic_1ms(void) { | ||
|
|
||
| // These functions need to be called at 1 kHz, or every 1ms | ||
| interface.rxClient(); |
| void CANModule::periodic_10ms(void) { | ||
|
|
||
| // These functions need to be called every 100 Hz (10ms) | ||
| interface.txProcessor(); |
| interface.rxClient(); | ||
| } | ||
|
|
||
| void CANModule::periodic_10ms(void) { |
There was a problem hiding this comment.
was there a discussion on tx being 10ms?
There was a problem hiding this comment.
RX runs at 1kHz and TX runs at 100Hz
There was a problem hiding this comment.
i more meant like was there a reason those numbers were chosen? its fine if there wasn't- they seem reasonable anyways
There was a problem hiding this comment.
Hmm there's no concrete reason, other than we want RX to run faster than TX
|
@upadhyaydhruv Actually, I thought about this a bit and I think it doesn't make sense to have both a CANModule and a CANInterface when the CANInterface is getting changed so that it can only be used in a module context. Can you delete the existing CANInterface driver and move all the logic into CANModule? Also, can you rename |
cindyli-13
force-pushed
the
app-structure-redesign
branch
from
1cf1f09 to
07b4016
Compare
* Add modules and main.cpp skeleton * Fix typos
* Watchdog module * clang fix * clang fix2 * test-watchdog * Edited WatchdogWrapper.h * Fixed pure virtual function errors * fixed namespace error * Fixed error2 * Cmake * Cmake * Cmake fix * Top-level Cmake * Changes made * clang fix * Update libs/utility/src/WatchdogWrapper.cpp Co-authored-by: Cindy Li <cindyli0213@hotmail.com> * Update test-apps/test-watchdog/src/main.cpp Co-authored-by: Cindy Li <cindyli0213@hotmail.com> * cmake * Changed periodic function name * watchdogwrapper fix Co-authored-by: Cindy Li <cindyli0213@hotmail.com>
* PDB Module added * CMake * rename include * Fixed Errors * clang fix * Changes made * Made pins config private members * clang
upadhyaydhruv
force-pushed
the
can_module_restructure
branch
from
bab7f0e to
6947dd6
Compare
4 participants
No description provided.