Fixed FOME CAN data bus selection #275
Conversation
| @@ -216,17 +216,18 @@ static void populateFrame(Odometry& msg) { | |||
| void sendCanVerbose() { | |||
| auto base = engineConfiguration->verboseCanBaseAddress; | |||
| auto isExt = engineConfiguration->rusefiVerbose29b; | |||
|
|
|||
| @@ -112,9 +112,9 @@ class CanTxTyped final : public CanTxMessage | |||
| }; | |||
|
|
|||
| template <typename TData> | |||
| void transmitStruct(uint32_t id, bool isExtended) | |||
| void transmitStruct(uint32_t id, bool isExtended, bool canChannel = false) | |||
There was a problem hiding this comment.
shouldn't this be a size_t ("bus")?
line 31:
explicit CanTxMessage(uint32_t eid, uint8_t dlc = 8, size_t bus = 0, bool isExtended = false);
There was a problem hiding this comment.
let's make this parameter canChannel non-optional - that's sort of what got us in this mess in the first place
There was a problem hiding this comment.
Should we update CanTxMessage::CanTxMessage so that inside the function we look at which bus to use?
CanTxMessage::CanTxMessage(uint32_t eid, uint8_t dlc, bool isExtended)Or do we want to changed transmitStruct so that we need to pass the bus to the function?
void transmitStruct(uint32_t id, bool isExtended, bool canChannel)There was a problem hiding this comment.
so that inside the function we look at which bus to use?
eh? All I mean by my comment was to remove = false on line 115 here.
There was a problem hiding this comment.
When I change that, I need to update all uses for transmitStruct
| transmitStruct<Aim5f5>(0x5f5, false); | ||
| transmitStruct<Aim5f6>(0x5f6, false); | ||
| transmitStruct<Aim5f7>(0x5f7, false); | ||
| auto canChannel = engineConfiguration->canBroadcastUseChannelTwo; |
There was a problem hiding this comment.
Is not "canChannel" a less than ideal name for "use second channel"? also what if soon more than two buses would be supported? A few of stm32f7 have three CAN buses.
There was a problem hiding this comment.
Now canBroadcastUseChannelTwo is a Bool, CAN 2 is a true / 1 so when more CAN buses are added we can switch to a int which keeps the logic the same.
0 -> CAN1
1 -> CAN2
2 -> CAN3
There was a problem hiding this comment.
So why not "size_t busIndex = engineConfiguration->canBroadcastUseChannelTwo;" to keep the... technical debt most localized?
There was a problem hiding this comment.
yea, that was going to be my next comment, we should really be doing size_t busIndex = engineConfiguration->canBroadcastUseChannelTwo ? 1 : 0
| @@ -91,7 +91,7 @@ class CanTxTyped final : public CanTxMessage | |||
| #endif // EFI_CAN_SUPPORT | |||
|
|
|||
| public: | |||
| explicit CanTxTyped(uint32_t id, bool isExtended) : CanTxMessage(id, sizeof(TData), isExtended) { } | |||
| explicit CanTxTyped(uint32_t id, bool isExtended, bool canChannel) : CanTxMessage(id, sizeof(TData), canChannel, isExtended) { } | |||
There was a problem hiding this comment.
bool to size_t conversion heh?
pass the CAN bus via the
transmitStructtoCanTxMessage::CanTxMessage, bus is by default 0, now looks at the setting from TS,bool canBroadcastUseChannelTwo.