Add Multiplex signal support. Fix big endian decoding. #46
Conversation
|
@bit-dream as in my pr comment I was not able to completely finish adding proper multiplex support. Would definitely need help on dbc adding / removing and dbc writing. |
There was a problem hiding this comment.
Hey, sorry for getting back to you late on this.
Overall, the changes look great. However, I think there are still some issues with the decode implementation, I don't think it's quite there yet/accurately decodes signals. I tried to be as detailed as I could with the comments, so check those out and let me know what you think.
That said, I would be willing to merge all of your changes into main, except for the changes to Can and BitUtils. If you want to open up a separate pull request for those other changes, I'll get those added, as I like what you did there.
As for the signal decoding logic, we may just need to revisit that. If you want me to take a stab at it I am happy too. If you want to take that on, you are welcome to as well.
src/can/BitUtils.ts
Outdated
| @@ -1,6 +1,17 @@ | |||
| import { EndianType } from '../shared/DataTypes'; | |||
|
|
|||
| class BitUtils { | |||
| static bigEndianBitOrder = [ | |||
There was a problem hiding this comment.
Right now this is probably fine, but I think we need some way of generating this array dynamically. The only reason I say that is due to the fact that DBC files support CAN FD. CAN FD can have message payloads of up to 64 bytes. If we go this route, someone who wants to decode a CAN FD frame will have invalid data returned, due to the function used below, indexOf(), which will return -1.
Technically the createFrame method in Can.ts checks for payloads > 8 bytes (and throws an error when it's bigger), so this probably doesn't break anything. However, the check for 8 bytes was a mistake on my part and not realizing CAN FD was supported in DBCs. So I had planned on removing that check.
There was a problem hiding this comment.
I wont even attempt to claim to properly understand how can and candfd bit ordering stuff works. I am unable to find good documentation on it, so I was just attempting solutions till it got what I expected.
But as far as can fd goes I not super worried about it right now, as this lib does not account for it yet, but when that feature is added checks and function will need to change for it.
src/can/BitUtils.ts
Outdated
| // Need to account for sawtooth bit numbering in CAN messages | ||
| startOfBit = endOfBitField - bitRange + 1; | ||
| // startOfBit = binary.length - (binary.length - startBit + bitRange); | ||
| startOfBit = BitUtils.bigEndianBitOrder.indexOf(startBit); |
There was a problem hiding this comment.
I have some initial concerns with this approach and I don't think this will actually extract the start bit we need to do the decoding.
This is kind of a follow-up discovery based on issue #45 where I was getting a start bit of 8 from your 11. The reason I got a value of 8 was that I made a quick DBC file from the signal definition you posted and loaded it into Vector's DBC editing tool. Apparently when Vector (and other tools I tested), the start bit is automatically adjusted if the endian type is set to Motorola. 11 turned into 8 to account for that (even though the signal definition states it as 11) [See Image].
The original start bit in the dbc file is retained and remains 11:
SG_ Service M : 11|4@0+ (1,0) [0|15] "" Vector__XXX
To me, this looks like the DBC file is intended to store the start bit as the Most Significant Bit (MSB) [11], and when the DBC file is parsed, the tool will convert that start bit to start at the Lowest Significant Bit (LSB) [8].
I modified the dbc file that you gave and added two additional signals that had lengths > 8 to see how the Vector tool handled the start bit discrepancies. Here is how all of the signals are laid out in the bit field (also note that one of your signals actually overlaps with the Service signal):
Here is the signals according to the DBC file:
BO_ 2147485672 OBD2: 8 Vector__XXX
SG_ TestSignal2 : 55|10@0+ (1,0) [0|0] "" Vector__XXX
SG_ TestSignal1 : 38|15@0+ (1,0) [0|0] "" Vector__XXX
SG_ S6base : 2|4@1+ (1,0) [0|0] "" Vector__XXX
SG_ S5base : 10|4@1+ (1,0) [0|0] "" Vector__XXX
SG_ S1_PID_0D_VehicleSpeed m13 : 31|8@0+ (1,0) [0|255] "km/h" Vector__XXX
SG_ S1_PID_11_ThrottlePosition m17 : 31|8@0+ (0.39216,0) [0|100] "%" Vector__XXX
SG_ SS1 m1M : 23|8@0+ (1,0) [0|255] "" Vector__XXX
SG_ Service M : 11|4@0+ (1,0) [0|15] "" Vector__XXX
So in terms of start bits, they are:
TestSignal2:
MSB: 55 LSB: 62
TestSignal1:
MSB: 38 LSB: 40
S1_PID_0D_VehicleSpeed:
MSB: 31 LSB: 24
S1_PID_11_ThrottlePosition:
MSB : 31 LSB: 24
SS1:
MSB: 23 LSB: 16
Service:
MSB: 11 LSB: 8
One thing to notice here is that when we do bigEndianBitOrder.indexOf(11) [start bit of the Service signal] we'll get a value of 12.
binary.slice() will have a range of [12, 16] whereas our extraction range needs to be: [8, 11].
This is further evident too when we look at the additional TestSignals that I added. TestSignal2 with the indexOf method will return a start bit of 48. Whereas the LSB should be 62.
TestSignal1 returns 33 (should be 40).
Etc.
I believe my method 8 * Math.floor(startBit / 8) + (7 - (startBit % 8)) is actually doing the same thing your indexOf method is (from the few I tested, it returns the same value). Just in a less hard coded way. I think both might be wrong though then?
I've been playing around with a way to convert MSB to LSB and here is a generic forumla to do that (can certainly be cleaned up) based on signal length and MSB:
LSB = (floor(((floor(msb/8)*8) + (7-(msb-(floor(msb/8)*8))) + (length-1))/8)*8) + (7-(((floor(msb/8)*8) + (7-(msb-(floor(msb/8)*8))) + (length-1))-(floor(((floor(msb/8)*8) + (7-(msb-(floor(msb/8)*8))) + (length-1))/8)*8)))
It seems to work for all of the conversions from your DBC I have above.
I think in general we just may need a different approach here. We can calculate the LSB using the equation above and we may need to iterate through each byte to extract the relevant bits. If you study the layout image I have above and the bit numbers of the individual signals, you might make the same realization that I am having where you can't just necessarily pull out the range of bits starting at the MSB or LSB. You need to account for that saw tooth pattern while doing the extraction.
Check out the implementation for signal unpacking I stumbled upon that MATLAB uses:
function value = unpack(obj, startBit, signalSize, byteOrder, dataType)
% unpack Unload a raw signal value from message data.
%
% VALUE = unpack(MESSAGE, STARTBIT, SIGNALSIZE, BYTEORDER, DATATYPE)
% takes a set of input parameters to unload raw signal VALUE(s) from the
% MESSAGE(s) and returns them as output.
%
% Inputs:
% MESSAGE - The CAN message(s) from which to unpack the signal(s).
% STARTBIT - The signal's starting bit location in the message data
% with a value between 0 and 63. The STARTBIT represents the
% least significant bit position in the CAN data of the signal.
% SIGNALSIZE - The signal's length in bits with a value between 1 and 64.
% BYTEORDER - The signal's endian format as 'LittleEndian' or
% 'BigEndian'. This is often referenced as Intel for
% 'LittleEndian' and Motorola for 'BigEndian'.
% DATATYPE - The desired type of the returned VALUE as 'int8',
% 'int16', 'int32', 'int64', 'uint8', 'uint16', 'uint32',
% 'uint64', 'single', or 'double'.
%
% Note that MESSAGE can be an array of CAN messages. When given as an
% array, the raw signal value is unpacked from each entry in the array
% individually. These signal values are then returned as an array of
% equal size to MESSAGE. The VALUE array is ordered the same as in the
% original MESSAGE array. Each MESSAGE in the array must have the
% same definition.
%
% Example:
% value = unpack(message, 56, 64, 'BigEndian', 'double')
%
% See also VNT.
% Check the argument count.
narginchk(5,5);
% Validate the argument startBit.
validateattributes(startBit, {'numeric'}, ...
{'finite', 'integer', 'nonempty', 'nonnan', 'nonnegative', ...
'nonsparse', 'real', 'scalar', ...
'<', (numel(obj(1).PrivateData) * 8)}, ...
'unpack', 'STARTBIT');
% Validate the argument signalSize.
validateattributes(signalSize, {'numeric'}, ...
{'finite', 'integer', 'nonempty', 'nonnan', 'positive', ...
'real', 'scalar', '<=', 64}, ...
'unpack', 'SIGNALSIZE');
% Validate the byteOrder argument.
byteOrder = validatestring(byteOrder, ...
{'LittleEndian', 'BigEndian'}, ...
'unpack', 'BYTEORDER');
% Validate the argument dataType.
dataType = validatestring(dataType, ...
{'uint8', 'int8', 'uint16', 'int16', 'uint32', 'int32', ...
'uint64', 'int64', 'single', 'double'}, ...
'unpack', 'DATATYPE');
% Initialize the return value to 0. Also make value an array of the
% appropriate size equal to the length of the provided message array.
value = zeros(1, numel(obj));
% Cast the type of the return value to an appropriately sized uint type
% so that the bit set and get functions will operate on it. Also, set a
% signed bit variable here depending on the type. The value represent
% the maximum number of bits in the data type for sign extension
% purposes. Zero indicates no sign bits are possible.
switch dataType
case 'uint8'
value = cast(value, 'uint8');
signBits = 0;
case 'int8'
value = cast(value, 'uint8');
signBits = 8;
case 'uint16'
value = cast(value, 'uint16');
signBits = 0;
case 'int16'
value = cast(value, 'uint16');
signBits = 16;
case {'uint32', 'single'}
value = cast(value, 'uint32');
signBits = 0;
case 'int32'
value = cast(value, 'uint32');
signBits = 32;
case {'uint64', 'double'}
value = cast(value, 'uint64');
signBits = 0;
case 'int64'
value = cast(value, 'uint64');
signBits = 64;
end
% Check the endian configuration.
bigEndian = strcmpi(byteOrder, 'BigEndian');
% Loop and extract the signal value for each message provided.
for i = 1:numel(obj)
% OPTIMIZATION: Store the Data property of the message object in a
% local variable to make access faster during the bit get operation.
tmpData = obj(i).PrivateData;
% Set the first target bit as the start bit. Add one to adjust
% indexing from zero based to one based.
targetBit = startBit + 1;
% Perform unpacking operations on each bit of the signal.
for j = 1:signalSize
try
% Determine the byte position and bit position within the
% byte to read.
targetByteIndex = ceil((targetBit) / 8);
targetBitIndex = (targetBit) - (targetByteIndex - 1) * 8;
% Get the bit from the message data.
bitValue = bitget(tmpData(targetByteIndex), targetBitIndex);
% Write the bit into the signal value if the bit is 1. We
% do not need to write 0 as the output value is initialized
% to 0 already.
if bitValue
value(i) = bitset(value(i), j, bitValue);
end
% Set the next target bit based on the byte ordering.
if bigEndian && (targetBitIndex == 8)
% When hitting a byte boundary, the target bit needs
% to jump across the boundary and through to the least
% significant bit of the next byte. This is a jump of
% 15 bits in length. This big jump is only for
% Big Endian values.
targetBit = targetBit - 15;
else
% Otherwise, when Little Endian or when not against a
% byte boundary, just index to the next bit in the
% byte.
targetBit = targetBit + 1;
end
catch err
% On error, the function tried a bit access that was not
% possible given the parameters of the signal.
error(message('vnt:Message:SignalDefinitionError'));
end
end
% When unpacking signed numbers that do no fill the size of the
% output data type container, the sign bit may need to be extended.
if signBits
% If the value of the most significant bit is 1, then extend it
% to the length of the storage container. We do not need to
% extend 0 as the bit values in the container are already 0.
if bitget(value(i), signalSize)
% Loop over the number of sign bits required.
for k = signBits:-1:(signalSize + 1)
% Set the bit to extend the sign.
value(i) = bitset(value(i), k, 1);
end
end
end
end
% Do a final typecast to the desired return type. Typecast is used to
% arrive at the proper type without changing the actual bit field.
value = typecast(value, dataType);
That shouldn't be an issue at all. I can work on adding that functionality. |
|
@bit-dream I removed the can and bit changes as requested. |
|
@agent6262 I approved your pull request and merged into main. Awesome work and thanks for contributing. I'll take a stab at implementing a more robust decode method. I'll update issue #45 so you can be in the loop when those changes get pulled in. Additionally, if you are looking to be an active maintainer, feel free to give me a shout. |
Fix for #44, #45 and #13. However this change is not fully featured as when adding and removing signals to the dbc they do not properly reflect in the object (also writing the db might not show multiplexing correctly?)