What a glorious muddle.
TX mappings are and have always been signed.
That said a TX mapping continues to overflow in a "useful" way to behave as if it was unsigned. So this new unit test I wrote passes just fine:
Code: Select all
static void send_map_le_full_scale_throttle_position_in_first_word()
{
canMap->AddSend(Param::pot, CanId, 0, 12, 1.0, 0);
Param::SetFloat(Param::pot, 4095);
canMap->SendAll();
ASSERT(FrameMatches({ 0xff, 0x0f, 0, 0, 0, 0, 0, 0 }));
}
RX mappings were sometimes signed and other times unsigned when they worked. I made them mostly signed except for single bit values which I special cased to be 1 or 0 (and I should have twigged something was wrong here). Two OI nodes with complementary TX and RX mappings should be able to interchange params in a way they could not previously.
This new unit test used to pass just fine but now fails (i.e. confirming that the previous behaviour was unsigned):
Code: Select all
static void receive_map_le_full_scale_throttle_position_in_first_word()
{
canMap->AddRecv(Param::pot, CanId, 0, 12, 1.0, 0);
SendFrame({ 0xff, 0x0f, 0, 0, 0, 0, 0, 0 });
ASSERT(Param::GetInt(Param::pot) == 4095);
}
I'm thinking about starting a new PR with:
- a safe subset of PR #8 avoiding signed mappings
- Big-endian TX mapping
- properly clamping TX mappings at 0..2^length. From inspection it looks like horrendous things can happen on overflow right now.
Then a further PR extending CAN mapping:
- Signed mappings
- Add a new SDO interface (as above)
- Extend the size of offset to be useful
- Proper extended CAN ID support
I will not be supporting multiple SDO interfaces in openinverter_can_tool at this point. Also, I will not be extending it to support the partial extended CAN frames. It's an experimental tool. Sorry.
I'm happy to add the code into libopeninv to support multiple interfaces if you wish it.
I would suggest that a "type" field would be a better way of encapsulating signed and unsigned. It would allow the addition of 32-bit float mappings as well. Looking at SavvyCAN that's certainly something that is found in the wild and would be simple to add at this point.
I prototyped a separate parameter for big-endian vs. little-endian. I found it confusing. The negative numbers for length work well when thinking about how the bits end up in the resulting CAN frame.
Does this seem a reasonable starting point? This would likely take the rest of the summer assuming I can work at this near full time as I have been recently.