Skip to content
Draft
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
created CANModule and fixed txProcessor function
  • Loading branch information
upadhyaydhruv committed Feb 4, 2022
commit d1c8689cf10bc8954630c11f84e8f50c9b3bfc4a
2 changes: 1 addition & 1 deletion libs/can/include/CANInterface.h
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ class CANInterface {
bool getRXSignalValue(HWBRIDGE::CANID msgID, HWBRIDGE::CANSIGNAL signalName, HWBRIDGE::CANSignalValue_t &signalValue);

// Switch CAN bus
bool switchCANBus(HWBRIDGE::CANBUSID canBusID);
bool switchCANBus(HWBRIDGE::CANBUSID canBusID); // TODO: Unnecessary

// Set CAN bus hw filter
bool setFilter(HWBRIDGE::CANFILTER filter, CANFormat format = CANStandard,
Expand Down
18 changes: 18 additions & 0 deletions libs/can/include/CANModule.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
#pragma onceusing

#include "Module.h"
#include "mbed.h"
#include "CANInterface.h"

class CANModule final : public Module {
public:

CANModule();

void periodic_1ms(void) override {}
void periodic_10ms(void) override {}


private:
CANInterface
}
78 changes: 37 additions & 41 deletions libs/can/src/CANInterface.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,8 @@ void CANInterface::rxClient(void) {
// Wait for a message to arrive
do {
mail = m_rxMailbox.try_get(); // using try_get() because try_get_for() was crashing
ThisThread::sleep_for(1ms);
} while (mail == nullptr);
ThisThread::sleep_for(1ms); // TODO: Check for object in mailbox every ms
} while (mail == nullptr);

MBED_ASSERT(mail != nullptr);
Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would the MBED_ASSERT not account for that?

Copy link
Member

Choose a reason for hiding this comment

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

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


Expand Down Expand Up @@ -112,51 +112,47 @@ void CANInterface::rxClient(void) {
}

void CANInterface::txProcessor(void) {
while (true) {
auto startTime = Kernel::Clock::now();
auto startTime = Kernel::Clock::now();
Copy link
Member

Choose a reason for hiding this comment

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

startTime is now no longer used here


CANMsg *mail = nullptr;
CANMsg *mail = nullptr;

// Send all one-shot messages that were queued
while ((mail = m_txMailboxOneShot.try_get()) != nullptr) {
if (!m_activeCANBus->write(*mail)) {
MBED_WARNING(MBED_MAKE_ERROR(MBED_MODULE_PLATFORM, MBED_ERROR_CODE_WRITE_FAILED), "CAN TX write failed");
m_numCANTXFaults++;
}
MBED_ASSERT(m_txMailboxOneShot.free(mail) == osOK);
ThisThread::sleep_for(TX_INTERDELAY);
// Send all one-shot messages that were queued
while ((mail = m_txMailboxOneShot.try_get()) != nullptr) {
if (!m_activeCANBus->write(*mail)) {
MBED_WARNING(MBED_MAKE_ERROR(MBED_MODULE_PLATFORM, MBED_ERROR_CODE_WRITE_FAILED), "CAN TX write failed");
m_numCANTXFaults++;
}
MBED_ASSERT(m_txMailboxOneShot.free(mail) == osOK);
ThisThread::sleep_for(TX_INTERDELAY);
Copy link
Member

Choose a reason for hiding this comment

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

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

}

// Send all streamed messages
if (m_txMsgMap != nullptr) {
for (auto it = m_txMsgMap->begin(); it != m_txMsgMap->end(); it++) {
HWBRIDGE::CANID msgID = it->first;
HWBRIDGE::CANMsgData_t msgData = {0};
size_t len = 0;

m_txMutex.lock();
bool msgPacked = HWBRIDGE::packCANMsg(msgData.raw, msgID, m_txMsgMap, len);
m_txMutex.unlock();

if (msgPacked) {
// Send message
CANMsg msg;
msg.setID(msgID);
msg.setPayload(msgData, len);
m_activeCANBus->write(msg);

m_numStreamedMsgsSent++;

ThisThread::sleep_for(TX_INTERDELAY);
} else {
MBED_WARNING(MBED_MAKE_ERROR(MBED_MODULE_PLATFORM, MBED_ERROR_CODE_INVALID_DATA_DETECTED),
"CAN TX message packing failed");
m_numCANTXFaults++;
}
// Send all streamed messages
if (m_txMsgMap != nullptr) {
for (auto it = m_txMsgMap->begin(); it != m_txMsgMap->end(); it++) {
HWBRIDGE::CANID msgID = it->first;
HWBRIDGE::CANMsgData_t msgData = {0};
size_t len = 0;

m_txMutex.lock();
bool msgPacked = HWBRIDGE::packCANMsg(msgData.raw, msgID, m_txMsgMap, len);
m_txMutex.unlock();

if (msgPacked) {
// Send message
CANMsg msg;
msg.setID(msgID);
msg.setPayload(msgData, len);
m_activeCANBus->write(msg);

m_numStreamedMsgsSent++;

ThisThread::sleep_for(TX_INTERDELAY);
Copy link
Member

Choose a reason for hiding this comment

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

ditto comment about sleeps in periodic

} else {
MBED_WARNING(MBED_MAKE_ERROR(MBED_MODULE_PLATFORM, MBED_ERROR_CODE_INVALID_DATA_DETECTED),
"CAN TX message packing failed");
m_numCANTXFaults++;
}
}

ThisThread::sleep_until(startTime + TX_PERIOD);
}
}

Expand Down
17 changes: 17 additions & 0 deletions rover-apps/common/include/CANModule.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
#pragma onceusing

#include "CANInterface.h"
#include "Module.h"
#include "mbed.h"

class CANModule final : public Module {
public:
CANModule(const Config &config);

void periodic_1ms(void) override {}
void periodic_10ms(void) override {}

private:
CANInterface interface;

};
21 changes: 21 additions & 0 deletions rover-apps/common/src/CANModule.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
#include "CANModule.h"

#include "Module.h"
#include "mbed.h"

// Using initializer lists to construct CANInterface object will attach ISRs and other required tasks
CANModule::CANModule(const Config &config) : interface(config) {
// Nothing should be required here
}

void CANModule::periodic_1ms(void) {

// These functions need to be called at 1 kHz, or every 1ms
interface.rxClient();
Copy link
Member

Choose a reason for hiding this comment

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

function should be renamed

}

void CANModule::periodic_10ms(void) {
Copy link
Member

Choose a reason for hiding this comment

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

was there a discussion on tx being 10ms?

Copy link
Member

Choose a reason for hiding this comment

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

RX runs at 1kHz and TX runs at 100Hz

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

@cindyli-13 cindyli-13 Jan 16, 2022

Choose a reason for hiding this comment

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

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


// These functions need to be called every 100 Hz (10ms)
interface.txProcessor();
Copy link
Member

Choose a reason for hiding this comment

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

function should be renamed

}