From decbdfb21705234771cd8f0d3f6bce16574e26f7 Mon Sep 17 00:00:00 2001 From: "David J. Fiddes" <35607151+davefiddes@users.noreply.github.com> Date: Sun, 23 Jun 2024 18:23:25 +0100 Subject: [PATCH] CAN mapping fixes (#18) * Enable signed gains when mapping via SDO It is possible with the serial terminal to configure negative gain parameters for a can rx or tx map entry. When reading back via CAN SDO the map entry's gain value is incorrect. When adding a can map entry via SDO the gain is interpreted as a positive value only. Tests: - Create can map entries via CAN SDO using openinverter_can_tool with a mix of : - gain values -8388.608, -1.0, 1.0 and 8388.607 - offset values -128, 0 and 127 - verify "oic can list" shows the map created - verify via serial terminal that "can p" shows the same can map - check can map operation transmits the expected behaviour for a negative gain. * Improve validation when adding can map entries Validate that the offset and length for a can map entry will no overflow the resulting CAN frame. So offset 63/length 1 is valid as is offset 32/length 32 but offset 60/length 8 is not. This change affects both serial and SDO. The length field must be at least 1 bit to make sense. This was already validated by the SDO configuration but ignored by serial. Tests: - Verify config of map entries with serial terminal fails as documented - Verify with a doctored openinverter_can_tool without client side validation of offset and length that failure is reported * Send mapped params which overlap first/second words It is possible to configure a CAN map entry which spans the internal 32-bit word boundary used to store can frames. When sending params to CAN only the bits in the first 32-bit word are actually sent. The fix involves detecting the overlap condition and shifting to the two words. It uses uint32_t consistently to avoid errors due to sign-extension when shifting. The receive path already handled the split word case. Tests: - Create 16, 24 and 32-bit parameter mappings at positions that overlap the 32- bit boundary. - Use positive and negative values in the mapped param - Verify with a custom DBC in SavvyCAN that the values are interpreted and scales appropriately. - Additionally, add mappings either side of the split mapping to verify no data overwrite * Fix parameters mapped to first 32-bits Correctly map params of 32-bit in size that fit entirely within the first word of the CAN frame. This fixes an integer overflow in the mask generation which doesn't matter for 32-bit ARM but is undefined behaviour and causes problems when compiled for x86-64. Tests: - Map a negative and positive valued params to offset 0, length 32 and verify the CAN frame contains the expected value in the first 4 bytes. * Fix validation of big-endian CAN map offsets In big-endian can mappings the offset specifies the end bit for the mapping and the negative length counts bits back. The existing offset validation only worked for little endian mappings where the values are all positive. This changes separates the validation. Tests: - Verify 32-bit mappings work with offsets of 31, 55, 63 - Verify 32-bit mappings fail with offsets of 7, 30 and 64 * Implement big-endian tx can mapping Big-endian transmit can mappings share the same masking as little-endian mappings. The bytes are then swapped before shifting into the appropirate position. The maths is different as the offset position in big-endian mappings specifies the end bit offset wheras little-endian it's the start bit. Tests: - Create 8-bit mappings with offset 7 and 39 - Create 16-bit mappings with offset 23, 39 and 47 - Create 24-bit mappings with offset 23, 55 and 63 - Create 32-bit mappings with offset 31, 39, 47, 55, 63 - Test each mapping with positive and negative values - Verify the output in SavvyCAN with equivalent Message/Signal * Fix CAN rx mapping for negative numbers Negative numbers are not received properly for can rx maps smaller than 32-bits. To fix this we need to sign extend values so they fit in an int32_t before converting to a float. The sign extension makes no sense for single bits as this would result in -1 being returned rather than 1 when the bit is asserted. Tests: - Create BE and LE 8-bit, 16-bit, 24-bit, 31-bit and 32-bit mappings - Verify with positive and negative numbers that SavvyCAN reports the expected value --- src/canmap.cpp | 84 ++++++++++++++++++++++++++++++++++++++++++-------- src/cansdo.cpp | 8 +++-- 2 files changed, 78 insertions(+), 14 deletions(-) diff --git a/src/canmap.cpp b/src/canmap.cpp index 89f82fe..d3fe749 100644 --- a/src/canmap.cpp +++ b/src/canmap.cpp @@ -80,7 +80,6 @@ bool CanMap::HandleRx(uint32_t canId, uint32_t data[2], uint8_t) uint32_t word; uint8_t pos = curPos->offsetBits; uint8_t numBits = ABS(curPos->numBits); - uint32_t mask = (1 << numBits) - 1; if (curPos->numBits < 0) //negative length is big endian { @@ -102,7 +101,7 @@ bool CanMap::HandleRx(uint32_t canId, uint32_t data[2], uint8_t) } //Swap byte order - uint8_t* bptr = (uint8_t*)&word; + const uint8_t* bptr = (uint8_t*)&word; word = (bptr[0] << 24) | (bptr[1] << 16) | (bptr[2] << 8) | bptr[3]; pos = 31 - pos; } @@ -113,7 +112,7 @@ bool CanMap::HandleRx(uint32_t canId, uint32_t data[2], uint8_t) word = data[1]; pos -= 32; //position in second word } - else if ((curPos->offsetBits + curPos->numBits) < 32) //all data in first word + else if ((curPos->offsetBits + curPos->numBits) <= 32) //all data in first word { word = data[0]; } @@ -125,7 +124,24 @@ bool CanMap::HandleRx(uint32_t canId, uint32_t data[2], uint8_t) } } - val = (word >> pos) & mask; + // extract the bits we are looking for + uint32_t mask = (1L << numBits) - 1; + word = (word >> pos) & mask; + + // sign-extend our arbitrary sized integer out to 32-bits but only if + // it is bigger than a single bit + int32_t ival; + if (numBits > 1) + { + uint32_t sign_bit = 1L << (numBits - 1); + ival = static_cast(((word + sign_bit) & mask)) - sign_bit; + } + else + { + ival = word; + } + + val = ival; val += curPos->offset; val *= curPos->gain; @@ -165,16 +181,50 @@ void CanMap::SendAll() val *= curPos->gain; val += curPos->offset; - int ival = val; - ival &= ((1 << curPos->numBits) - 1); + // convert to a signed integer value before storing in an unsigned to + // avoid sign-extension problems when we start shifting and masking + uint32_t ival = (int32_t)val; + uint8_t numBits = ABS(curPos->numBits); + ival &= (1UL << numBits) - 1; - if (curPos->offsetBits > 31) + if (curPos->numBits < 0) // big-endian { - data[1] |= ival << (curPos->offsetBits - 32); + //Swap byte order + const uint8_t* bptr = (uint8_t*)&ival; + ival = (bptr[0] << 24) | (bptr[1] << 16) | (bptr[2] << 8) | bptr[3]; + + if (curPos->offsetBits < 32) //all data in first word + { + data[0] |= ival >> (31 - curPos->offsetBits); + } + else if ((curPos->offsetBits + curPos->numBits) >= 31) //all data in second word + { + data[1] |= ival >> (63 - curPos->offsetBits); + } + else //data spans across both words + { + data[0] |= ival << (curPos->offsetBits - 31); + data[1] |= ival >> (63 - curPos->offsetBits); + } } - else + else // little-endian { - data[0] |= ival << curPos->offsetBits; + if (curPos->offsetBits > 31) + { + // data entirely in the second word + data[1] |= ival << (curPos->offsetBits - 32); + } + else if ((curPos->offsetBits + curPos->numBits) <= 32) + { + // data entirely in the first word + data[0] |= ival << curPos->offsetBits; + } + else + { + // data spans both words + data[0] |= ival << curPos->offsetBits; + data[1] |= ival >> (32 - curPos->offsetBits); + } } } @@ -453,8 +503,18 @@ void CanMap::ClearMap(CANIDMAP *canMap) int CanMap::Add(CANIDMAP *canMap, Param::PARAM_NUM param, uint32_t canId, uint8_t offsetBits, int8_t length, float gain, int8_t offset) { if (canId > MAX_COB_ID) return CAN_ERR_INVALID_ID; - if (offsetBits > 63) return CAN_ERR_INVALID_OFS; - if (length > 32 || length < -32) return CAN_ERR_INVALID_LEN; + if (length == 0 || ABS(length) > 32) return CAN_ERR_INVALID_LEN; + if (length > 0) + { + // little-endian mapping + if (offsetBits + length - 1 > 63) return CAN_ERR_INVALID_OFS; + } + else + { + // big-endian mapping + if (offsetBits > 63) return CAN_ERR_INVALID_OFS; + if (static_cast(offsetBits) + length + 1 < 0) return CAN_ERR_INVALID_OFS; + } CANIDMAP *existingMap = FindById(canMap, canId); diff --git a/src/cansdo.cpp b/src/cansdo.cpp index 08fcc01..9c444a6 100644 --- a/src/cansdo.cpp +++ b/src/cansdo.cpp @@ -330,7 +330,7 @@ void CanSdo::ReadOrDeleteCanMap(CAN_SDO* sdo) else if (sdo->subIndex & 1) //odd sub indexes have data id, position and length sdo->data = id | (canPos->offsetBits << 16) | (canPos->numBits << 24); else //even sub indexes except 0 have gain and offset - sdo->data = (((uint32_t)(canPos->gain * 1000)) & 0xFFFFFF) | (canPos->offset << 24); + sdo->data = (uint32_t)(((int32_t)(canPos->gain * 1000)) & 0xFFFFFF) | (canPos->offset << 24); sdo->cmd = SDO_READ_REPLY; } else @@ -379,7 +379,11 @@ void CanSdo::AddCanMap(CAN_SDO* sdo, bool rx) else if (mapInfo.numBits != 0 && sdo->subIndex == 2) //This sort of verifies that we received subindex 1 { //Now we receive gain and offset and add the map - mapInfo.gain = (sdo->data & 0xFFFFFF) / 1000.0f; + + // sign extend the 24-bit integer to a 32-bit integer + int32_t gainFixedPoint = (sdo->data & 0xFFFFFF) << (32-24); + gainFixedPoint >>= (32-24); + mapInfo.gain = gainFixedPoint / 1000.0f; mapInfo.offset = sdo->data >> 24; if (rx) //RX map