Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ namespace isobus
std::uint64_t timestampOffset; ///< The offset of the timestamps
NTCAN_HANDLE handle = NTCAN_NO_HANDLE; ///< The handle as defined in the NTCAN driver API
NTCAN_RESULT openResult = NTCAN_SUCCESS; ///< Stores the result of the call to begin CAN communication. Used for is_valid check later.
std::uint64_t numLostMsgs = 0; ///< total number of lost RX messages (due to RX FIFO overrun in hardware or driver)
};
}

Expand Down
151 changes: 120 additions & 31 deletions hardware_integration/src/ntcan_plugin.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,9 @@ namespace isobus
{
NTCANPlugin::NTCANPlugin(int channel, int baudrate) :
net(channel),
baudrate(baudrate)
baudrate(baudrate),
timestampFreq(1000000),
timestampOffset(0)
{
}

Expand All @@ -47,9 +49,9 @@ namespace isobus
isobus::CANStackLogger::error("[NTCAN]: Attempting to open a connection that is already open");
}
std::uint32_t mode = 0;
std::int32_t txQueueSize = 8;
std::int32_t rxQueueSize = 8;
std::int32_t txTimeOut = 100;
std::int32_t txQueueSize = min( NTCAN_MAX_TX_QUEUESIZE, 256 );
std::int32_t rxQueueSize = min( NTCAN_MAX_RX_QUEUESIZE, 256 );
Comment on lines +52 to +53
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit - I think this is std::min? So prefer explicit namespace

Suggested change
std::int32_t txQueueSize = min( NTCAN_MAX_TX_QUEUESIZE, 256 );
std::int32_t rxQueueSize = min( NTCAN_MAX_RX_QUEUESIZE, 256 );
std::int32_t txQueueSize = std::min( NTCAN_MAX_TX_QUEUESIZE, 256 );
std::int32_t rxQueueSize = std::min( NTCAN_MAX_RX_QUEUESIZE, 256 );

std::int32_t txTimeOut = 1000;
std::int32_t rxTimeOut = 1000;

openResult = canOpen(net, mode, txQueueSize, rxQueueSize, txTimeOut, rxTimeOut, &handle);
Expand Down Expand Up @@ -80,11 +82,17 @@ namespace isobus

if (NTCAN_FEATURE_TIMESTAMP == (status.features & NTCAN_FEATURE_TIMESTAMP))
{
isobus::CANStackLogger::debug("[NTCAN]: have timestamp feature");
std::uint64_t timestamp = 0;
openResult = canIoctl(handle, NTCAN_IOCTL_GET_TIMESTAMP_FREQ, &timestampFreq);
if (NTCAN_SUCCESS == openResult)
{
openResult = canIoctl(handle, NTCAN_IOCTL_GET_TIMESTAMP, &timestamp);
if (NTCAN_SUCCESS != openResult) {
isobus::CANStackLogger::error("[NTCAN]: Error NTCAN_IOCTL_GET_TIMESTAMP failed");
}
} else {
isobus::CANStackLogger::error("[NTCAN]: Error NTCAN_IOCTL_GET_TIMESTAMP_FREQ failed");
}
if (NTCAN_SUCCESS == openResult)
{
Expand All @@ -95,25 +103,67 @@ namespace isobus
}
}

std::int32_t ids = (1 << 11);
openResult = canIdRegionAdd(handle, 0, &ids);
if (NTCAN_SUCCESS == openResult && ids != (1 << 11))
bool smart_filt = true;
if (NTCAN_FEATURE_SMART_ID_FILTER != (status.features & NTCAN_FEATURE_SMART_ID_FILTER))
{
openResult = NTCAN_INSUFFICIENT_RESOURCES;
isobus::CANStackLogger::error("[NTCAN]: Error trying to add the standard ID region");
close();
return;
isobus::CANStackLogger::debug("[NTCAN]: do not have Smart ID Filter feature");
smart_filt = false;
}

if (smart_filt) {
std::int32_t ids = (1 << 11);
openResult = canIdRegionAdd(handle, 0, &ids);
if (NTCAN_SUCCESS != openResult || (NTCAN_SUCCESS == openResult && ids != (1 << 11)))
{
openResult = NTCAN_INSUFFICIENT_RESOURCES;
isobus::CANStackLogger::error("[NTCAN]: Error trying to add the standard ID region");
close();
return;
}
ids = (1 << 29);
openResult = canIdRegionAdd(handle, NTCAN_20B_BASE, &ids);
if (NTCAN_SUCCESS != openResult || (NTCAN_SUCCESS == openResult && ids != (1 << 29)))
{
openResult = NTCAN_INSUFFICIENT_RESOURCES;
isobus::CANStackLogger::error("[NTCAN]: Error trying to add the extended ID region");
close();
return;
}

ids = (1 << 29);
openResult = canIdRegionAdd(handle, NTCAN_20B_BASE, &ids);
if (NTCAN_SUCCESS == openResult && ids != (1 << 29))
{
openResult = NTCAN_INSUFFICIENT_RESOURCES;
isobus::CANStackLogger::error("[NTCAN]: Error trying to add the extended ID region");
close();
return;
} else {
// cannot use canIdRegionAdd() with old esd devices/drivers like "CAN-USB" first gen
std::int32_t id;
for (id = 0; id < 0x7FF; id++) {
openResult = canIdAdd(handle, id);
if(openResult != NTCAN_SUCCESS) {
openResult = NTCAN_INSUFFICIENT_RESOURCES;
isobus::CANStackLogger::error("[NTCAN]: Error trying to add the standard ID region (no SmartId filter)");
close();
return;
}
}
// Without SmartId filter: As soon as an arbitrary 29-bit CAN-ID is enabled with canIdAdd()
// all 29-bit CAN-IDs will pass the first filter stage, bcs. AMR register is default initialized
// to 0x1FFFFFFF.
id = NTCAN_20B_BASE;
openResult = canIdAdd(handle, id);
if(openResult != NTCAN_SUCCESS) {
openResult = NTCAN_INSUFFICIENT_RESOURCES;
isobus::CANStackLogger::error("[NTCAN]: Error trying to add the extended ID region (no SmartId filter)");
close();
return;
}
}

if(1) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can probably get rid of this if(1) condition check?

std::int32_t id = NTCAN_EV_CAN_ERROR; // canReadT() will report error events
NTCAN_RESULT res = canIdAdd(handle, id);
if(res != NTCAN_SUCCESS) {
isobus::CANStackLogger::warn("[NTCAN]: failed to enable CAN error event reporting");
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: We're in the isobus namespace here I think, so can probably omit it? Keeps the lines shorter.

Suggested change
isobus::CANStackLogger::warn("[NTCAN]: failed to enable CAN error event reporting");
CANStackLogger::warn("[NTCAN]: failed to enable CAN error event reporting");

There are other places in this file that do the same thing - I'd suggest updating them all if removing that prefix compiles alright

}
}
//#define NTCAN_IS_EVENT(id)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
//#define NTCAN_IS_EVENT(id)


}

bool NTCANPlugin::read_frame(isobus::CANMessageFrame &canFrame)
Expand All @@ -125,33 +175,72 @@ namespace isobus

result = canReadT(handle, &msgCanMessage, &count, nullptr);

if (NTCAN_SUCCESS == result && 1 == count)
{
canFrame.dataLength = msgCanMessage.len;
memcpy(canFrame.data, msgCanMessage.data, msgCanMessage.len);
canFrame.identifier = (msgCanMessage.id & ((1 << 29) - 1));
canFrame.isExtendedFrame = (NTCAN_20B_BASE == (msgCanMessage.id & NTCAN_20B_BASE));
if (NTCAN_SUCCESS != result || 1 != count) {
std::this_thread::sleep_for(std::chrono::milliseconds(1));
return false;
}

if ( NTCAN_IS_EVENT(msgCanMessage.id) ) {
// got a an event frame
EVMSG_T *msgCanEvent = (EVMSG_T*)&msgCanMessage;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This reinterpret cast is fairly nasty and feels like it might not be cross-platform (endianness) safe, but we can at least make that clearer by showing it as:

Suggested change
EVMSG_T *msgCanEvent = (EVMSG_T*)&msgCanMessage;
EVMSG_T *msgCanEvent = reinterpret_cast<EVMSG_T*>(&msgCanMessage);

std::uint32_t evt_len = msgCanEvent->len & 0x0F;
switch (msgCanEvent->evid) {
case NTCAN_EV_CAN_ERROR:
{
const EV_CAN_ERROR &err(msgCanEvent->evdata.error);
// uint8_t err.can_status; // CAN controller status
// uint8_t err.dma_stall; // DMA stall counter (HW dependent)
// uint8_t err.ctrl_overrun; // Controller overruns
// uint8_t err.fifo_overrun; // Driver FIFO overruns
}
break;
default:
; // ignore
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit - Since there's only 1 case, this can be simplified down to an if check

}
if (1) { // print EVT message
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can probably get rid of this if(1) condition check?

NTCAN_FORMATEVENT_PARAMS par = { 0 };
par.timestamp = msgCanEvent->timestamp;
par.timestamp_freq = timestampFreq;
par.num_baudrate = baudrate; // ???
char evt_msg[128];
evt_msg[0] = '\0';
result = canFormatEvent( (EVMSG*)msgCanEvent, &par, evt_msg, sizeof evt_msg);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Another yikesy C style cast here... I'd suggest either being clear that it's a reinterpret_cast or avoid the cast

if (NTCAN_SUCCESS == result) {
evt_msg[sizeof evt_msg -1] ='\0';
isobus::CANStackLogger::warn("[NTCAN EVT]: %s", evt_msg);
}
}
// no sleep here!
return false;
} else {
Copy link
Copy Markdown
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 this file will pass our style linter... before we approve it, can you format it using clang-format?

See our contributing guide: find . -iname *.hpp -o -iname *.cpp | xargs clang-format -i

// got a data frame, might be CC or FD
canFrame.dataLength = NTCAN_LEN_TO_DATASIZE(msgCanMessage.len);
memcpy(canFrame.data, msgCanMessage.data, canFrame.dataLength);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: namespacing

Suggested change
memcpy(canFrame.data, msgCanMessage.data, canFrame.dataLength);
std::memcpy(canFrame.data, msgCanMessage.data, canFrame.dataLength);

canFrame.identifier = NTCAN_ID(msgCanMessage.id); // lower 29 bits
canFrame.isExtendedFrame = NTCAN_IS_EFF(msgCanMessage.id) ? 1 : 0;
canFrame.timestamp_us = msgCanMessage.timestamp * 1000000 / timestampFreq + timestampOffset;
if (msgCanMessage.msg_lost > 0) {
numLostMsgs += msgCanMessage.msg_lost;
}
retVal = true;
}
else
{
std::this_thread::sleep_for(std::chrono::milliseconds(1));
}
return retVal;
}

bool NTCANPlugin::write_frame(const isobus::CANMessageFrame &canFrame)
{
NTCAN_RESULT result;
CMSG_T msgCanMessage{ 0 };
CMSG msgCanMessage{ 0 };
std::int32_t count = 1;

msgCanMessage.id = canFrame.isExtendedFrame ? (canFrame.identifier | NTCAN_20B_BASE) : canFrame.identifier;
msgCanMessage.len = canFrame.dataLength;
memcpy(msgCanMessage.data, canFrame.data, canFrame.dataLength);

result = canWriteT(handle, &msgCanMessage, &count, nullptr);
// we won't use canWriteT() here bcs. we do not need scheduled transmits: AND THERE IS A BUG IN
// current NTCAN driver: 'count' is returned 0 always while the CAN message has been send out
// successfully ==> this leads to 100% bus load bcs. the message will NOT be messagesToBeTransmittedQueue.pop()'ed
result = canWrite(handle, &msgCanMessage, &count, nullptr);

return (NTCAN_SUCCESS == result && 1 == count);
}
Expand Down