CANModule to fit CAN into app restructure #407

upadhyaydhruv · 2022-01-15T21:54:19Z

No description provided.

wmmc88

some of the functions you modified were tied to mbed threads. u need to remove the start calls to those threads in CANInterface.cpp

wmmc88 · 2022-01-16T16:19:25Z

libs/can/src/CANInterface.cpp

@@ -112,51 +108,47 @@ void CANInterface::rxClient(void) {
 }

 void CANInterface::txProcessor(void) {
-  while (true) {
-    auto startTime = Kernel::Clock::now();
+  auto startTime = Kernel::Clock::now();


startTime is now no longer used here

wmmc88 · 2022-01-16T16:23:38Z

libs/can/src/CANInterface.cpp

@@ -55,12 +55,8 @@ void CANInterface::rxPostman(void) {
 void CANInterface::rxClient(void) {
  while (true) {


should no longer be while true if its a 1ms periodic function

wmmc88 · 2022-01-16T16:27:06Z

libs/can/src/CANInterface.cpp

-      ThisThread::sleep_for(1ms);
-    } while (mail == nullptr);
+    // Check if a message has arrived:
+    mail = m_rxMailbox.try_get(); 

    MBED_ASSERT(mail != nullptr);


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)

Would the MBED_ASSERT not account for that?

MBED_ASSERT would actually break the program, I agree that we should remove it and just wrap the logic in an if block

wmmc88 · 2022-01-16T16:28:22Z

libs/can/src/CANInterface.cpp

    }
+    MBED_ASSERT(m_txMailboxOneShot.free(mail) == osOK);
+    ThisThread::sleep_for(TX_INTERDELAY);


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

wmmc88 · 2022-01-16T16:29:00Z

libs/can/src/CANInterface.cpp

+
+        m_numStreamedMsgsSent++;
+
+        ThisThread::sleep_for(TX_INTERDELAY);


ditto comment about sleeps in periodic

wmmc88 · 2022-01-16T16:29:32Z

rover-apps/common/src/CANModule.cpp

+void CANModule::periodic_1ms(void) {
+
+    // These functions need to be called at 1 kHz, or every 1ms
+    interface.rxClient();


function should be renamed

wmmc88 · 2022-01-16T16:29:35Z

rover-apps/common/src/CANModule.cpp

+void CANModule::periodic_10ms(void) {
+
+    // These functions need to be called every 100 Hz (10ms)
+    interface.txProcessor();


function should be renamed

wmmc88 · 2022-01-16T16:30:37Z

rover-apps/common/src/CANModule.cpp

+    interface.rxClient();
+}
+
+void CANModule::periodic_10ms(void) {


was there a discussion on tx being 10ms?

RX runs at 1kHz and TX runs at 100Hz

i more meant like was there a reason those numbers were chosen? its fine if there wasn't- they seem reasonable anyways

Hmm there's no concrete reason, other than we want RX to run faster than TX

cindyli-13 · 2022-01-16T17:33:25Z

@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 CANModule to CANDriverModule?

* 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

wmmc88 requested changes Jan 16, 2022

View reviewed changes

cindyli-13 force-pushed the app-structure-redesign branch from 1cf1f09 to 07b4016 Compare January 16, 2022 19:59

Base automatically changed from app-structure-redesign to master January 18, 2022 01:46

cindyli-13 and others added 10 commits February 3, 2022 21:49

Create modules skeleton for new app structure (#382)

b489be4

* Add modules and main.cpp skeleton * Fix typos

Create PDB monitoring module (#392)

1a488c7

* PDB Module added * CMake * rename include * Fixed Errors * clang fix * Changes made * Made pins config private members * clang

fix folder structure

4de2cff

fix common modules not found

3f052b1

created CANModule and fixed txProcessor function

d1c8689

fixed rxClient function for modular use

5de8de9

made CMake linking changes for CANModule

2c23625

implemented most changes as requested by Melvion and Cindy

541583d

added all changes requested, hw bridge issue persists

6947dd6

upadhyaydhruv force-pushed the can_module_restructure branch from bab7f0e to 6947dd6 Compare February 4, 2022 02:52

upadhyaydhruv added 2 commits February 10, 2022 17:59

fixed issues preventing compilation

a667ffa

added wrapper for other CANInterface functions

a0d658b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CANModule to fit CAN into app restructure #407

CANModule to fit CAN into app restructure #407

upadhyaydhruv commented Jan 15, 2022

wmmc88 left a comment

wmmc88 Jan 16, 2022

wmmc88 Jan 16, 2022

wmmc88 Jan 16, 2022

upadhyaydhruv Feb 3, 2022

cindyli-13 Feb 3, 2022

wmmc88 Jan 16, 2022

wmmc88 Jan 16, 2022

wmmc88 Jan 16, 2022

wmmc88 Jan 16, 2022

wmmc88 Jan 16, 2022

cindyli-13 Jan 16, 2022

wmmc88 Jan 16, 2022

cindyli-13 Jan 16, 2022 •

edited

Loading

cindyli-13 commented Jan 16, 2022

		@@ -55,12 +55,8 @@ void CANInterface::rxPostman(void) {
		void CANInterface::rxClient(void) {
		while (true) {


		m_numStreamedMsgsSent++;

		ThisThread::sleep_for(TX_INTERDELAY);

CANModule to fit CAN into app restructure #407

Are you sure you want to change the base?

CANModule to fit CAN into app restructure #407

Conversation

upadhyaydhruv commented Jan 15, 2022

wmmc88 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cindyli-13 Jan 16, 2022 • edited Loading

Choose a reason for hiding this comment

cindyli-13 commented Jan 16, 2022

cindyli-13 Jan 16, 2022 •

edited

Loading