NTCAN hardware_integration enhancement plus bugfix#672
NTCAN hardware_integration enhancement plus bugfix#672norbertmm wants to merge 2 commits intoOpen-Agriculture:mainfrom
Conversation
Fix for older esd CAN interfaces without SmartFilter feature and fix canWriteT() endless loop
ad3154
left a comment
ad3154
left a comment
There was a problem hiding this comment.
Thanks for this contribution!
I let a few comments mostly around some style things that could make the file a bit more consistent with the rest of the project and a bit tidier.
Since I don't have an NTCAN things to test with, folks like you who do are greatly appreciated!
| std::int32_t txQueueSize = min( NTCAN_MAX_TX_QUEUESIZE, 256 ); | ||
| std::int32_t rxQueueSize = min( NTCAN_MAX_RX_QUEUESIZE, 256 ); |
There was a problem hiding this comment.
Nit - I think this is std::min? So prefer explicit namespace
| 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 ); |
| } | ||
| } | ||
|
|
||
| if(1) { |
There was a problem hiding this comment.
Can probably get rid of this if(1) condition check?
| isobus::CANStackLogger::warn("[NTCAN]: failed to enable CAN error event reporting"); | ||
| } | ||
| } | ||
| //#define NTCAN_IS_EVENT(id) |
There was a problem hiding this comment.
| //#define NTCAN_IS_EVENT(id) |
| } | ||
| break; | ||
| default: | ||
| ; // ignore |
There was a problem hiding this comment.
Nit - Since there's only 1 case, this can be simplified down to an if check
| default: | ||
| ; // ignore | ||
| } | ||
| if (1) { // print EVT message |
There was a problem hiding this comment.
Can probably get rid of this if(1) condition check?
| } | ||
| // no sleep here! | ||
| return false; | ||
| } else { |
There was a problem hiding this comment.
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
| } else { | ||
| // got a data frame, might be CC or FD | ||
| canFrame.dataLength = NTCAN_LEN_TO_DATASIZE(msgCanMessage.len); | ||
| memcpy(canFrame.data, msgCanMessage.data, canFrame.dataLength); |
There was a problem hiding this comment.
Nit: namespacing
| memcpy(canFrame.data, msgCanMessage.data, canFrame.dataLength); | |
| std::memcpy(canFrame.data, msgCanMessage.data, canFrame.dataLength); |
| 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"); |
There was a problem hiding this comment.
Nit: We're in the isobus namespace here I think, so can probably omit it? Keeps the lines shorter.
| 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
|
|
||
| if ( NTCAN_IS_EVENT(msgCanMessage.id) ) { | ||
| // got a an event frame | ||
| EVMSG_T *msgCanEvent = (EVMSG_T*)&msgCanMessage; |
There was a problem hiding this comment.
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:
| EVMSG_T *msgCanEvent = (EVMSG_T*)&msgCanMessage; | |
| EVMSG_T *msgCanEvent = reinterpret_cast<EVMSG_T*>(&msgCanMessage); |
| par.num_baudrate = baudrate; // ??? | ||
| char evt_msg[128]; | ||
| evt_msg[0] = '\0'; | ||
| result = canFormatEvent( (EVMSG*)msgCanEvent, &par, evt_msg, sizeof evt_msg); |
There was a problem hiding this comment.
Another yikesy C style cast here... I'd suggest either being clear that it's a reinterpret_cast or avoid the cast
2 participants
Describe your changes
NTCAN Hardware integration (driver) does not work with CAN-USB/2 device from esd electronics gmbh.
Two issues:
Both issues fixed
by this change and tested successfully with CAN-USB/2 and also EtherCAN/3-FD devices using the SeederExample and a Trimble XCN-1050 Terminal.