Closed Uight closed 1 week ago
I now created a fork and branch to test this and it turns out the method above is incredibly slow compared to the original method. I didnt do a test with the .net benchmark but some measurements with the stopwatch. While the original method takes around 0.18ms the code above took 3.2ms. Therefor i did change to code in my branch around a bit and its now at around 0.21ms. Which would be good enough in my opinion.
While implementing i wonderd why theres a difference between RxSignalUnpack() and RxStateUnpack you could do pretty much just use the RxSignalUnpack base it on ulong and if it is signed do an unsafe cast to long like this: unchecked((long)ulongValue) and then scale that. You loose some precision with that (around 5 digits at the end) but i dont think thats a problem and in fact if someone uses a full 64 bit signed you would also loose the last 4 digits in precision which is why ist a bit unclear why theres to methods. Because the state unpack is only usefull if you have an unsigned int longer than 56bits or so. In the one implementation i know of this (Göpel electronic) im pretty sure you always get an double back.
Thanks @Uight for bringing CAN FD into discussion. Your insights are valuable and maybe it's time to implement support for CAN FD. I think using an array like others are doing (peak, etc.) is the way to go. Please feel free to continue to upgrade the unpack , pack functions to CAN FD, and if you make a pull request, we can work on that. Next week I will try to have a look as well.
@EFeru ill create a pull request when i finished the my changes. I Just switched the code to be based on ulong and until now the unitTest that are allready present are all running. Now with that code change you can see that the RxStateUnpack und the normal unpack are pretty much the same. Can you tell me what the RxStateUnpack is supposed to be used for. What do you think of the change?
For Testing the new unpack function based on arrays i would pretty much just use the existing testcases but cast the testdata to an byte array and repeat the test. And probably two more testcases one with a shorter byte array and one with a longer one.
For the Pack function based on a byte array i guess its way harder then the unpack. ill probably not do it anytime soon depends on when i would actually need it.
@EFeru i havent yet worked with the pack method but i just checked it out and i must say that i dont particulary like the interface.
What i mean is that for a function working on byte[] array i would define an interface like this: public static void PackSignal(ref byte[] frameData, Signal signal, double value) i need ref because i dont want to reassigne byte ararys all the time. with the current method you can do this (from readme): ulong TxMsg = 0; TxMsg |= Packer.TxSignalPack(value1, sig1); TxMsg |= Packer.TxSignalPack(value2, sig2); TxMsg |= Packer.TxSignalPack(value3, sig3);
Its nice that this is possible but its not obvios from the interface and id like to change it to a ref too just to keep both similar.
//the ref is not needed for the new byte[] method as byte[] is a reference type but if you would like to have the same interface for both methods then i suppose using ref in both cases (its needed with the int method) would be nice
From a doing perspective it shouldnt be as hard as i first thought with a code like this (more or less pseudo code):
public class CanMessagePacker
{
public static byte[] PackMessage(CanSignal[] signals, int messageLength)
{
byte[] message = new byte[messageLength];
foreach (var signal in signals)
{
PackSignal(message, signal);
}
return message;
}
private static void PackSignal(byte[] message, CanSignal signal)
{
int startBit = signal.StartBit;
int length = signal.Length;
uint value = signal.Value;
for (int bitIndex = 0; bitIndex < length; bitIndex++)
{
int bitPosition = startBit + bitIndex;
int byteIndex = bitPosition / 8;
int bitInBytePosition = bitPosition % 8;
// Extract the bit from the signal value
uint bitValue = (value >> bitIndex) & 1;
// Set the bit in the message
message[byteIndex] |= (byte)(bitValue << bitInBytePosition);
}
}
}
@Uight @EFeru I also encountered issues with CAN FD messages, and the advice I sought from AI is as follows, which I think is feasible. It should return uint64_T[].Of course, I need to make some modifications to the Signal class.
public static uint64_T[] TxSignalPack(double value, Signal signal)
{
uint64_T[] packedFrames = new uint64_T[signal.DataFrames.Length];
uint64_T bitMask = signal.BitMask();
// Apply scaling
var rawValue = (value - signal.Offset) / signal.Factor;
int64_T iVal;
if (signal.ValueType == DbcValueType.IEEEFloat)
iVal = (long)FloatConverter.AsInteger((float)rawValue);
else if (signal.ValueType == DbcValueType.IEEEDouble)
iVal = DoubleConverter.AsInteger(rawValue);
else
iVal = (int64_T)Math.Round(rawValue);
// Apply overflow protection
if (signal.ValueType == DbcValueType.Signed)
iVal = CLAMP(iVal, -(int64_T)(bitMask >> 1) - 1, (int64_T)(bitMask >> 1));
else if (signal.ValueType == DbcValueType.Unsigned)
iVal = CLAMP(iVal, 0L, (int64_T)bitMask);
// Manage sign bit (if signed)
if (signal.ValueType == DbcValueType.Signed && iVal < 0)
{
iVal += (int64_T)(1UL << signal.Length);
}
// Pack signal into each data frame
for (int i = 0; i < signal.DataFrames.Length; i++)
{
uint64_T startBit = signal.DataFrames[i].StartBit;
uint64_T length = signal.DataFrames[i].Length;
uint64_T frameMask = signal.DataFrames[i].FrameMask;
// Calculate the part of the signal value that should be packed into this frame
int64_T frameValue = (iVal >> startBit) & frameMask;
// Pack the frame value into the frame data
packedFrames[i] = (((uint64_T)frameValue & frameMask) << startBit);
}
// Combine the packed frames into a single CAN FD message
// This part is specific to the CAN FD protocol and may require additional logic
return packedFrames;
}
@lihaiyonggood just out of curiosity would you like the can fd methods being based on uint64 aswell? because for can fd there are some lengths that dont really work with uint64: 12, 20, in particular. You would need to cut the send data to specific length anyway. just as with normal can length under 8 bytes. What would be your preffered interface?
@Uight Indeed, I believe the best approach is to be able to pass all the Signals under the Message and convert them all at once into a CAN FD frame (byte array). This avoids the need for users to manually merge the values of the Signals and also eliminates the need for users to convert to a byte array themselves. Similarly, users can also convert all the physical values of the Signals from a CAN FD frame at once, rather than doing it one by one. As you know, some Messages may contain a large number of Signals, and manual operations by the user can be very cumbersome. You can refer to the cantools library in Python; I think its handling method is worth emulating. https://github.com/cantools/cantools
@Uight First, I modify GetStartBitLE(), return UInt16 object, CANFD message max bit length is 8*64=512.
/// <summary>
/// Get start bit Little Endian
/// </summary>
private static UInt16 GetStartBitLE(Signal signal, int messageByteCount = 8)
{
UInt16 startByte = (UInt16)(signal.StartBit / 8);
return (UInt16)(8 * messageByteCount - (signal.Length + 8 * startByte + (8 * (startByte + 1) - (signal.StartBit + 1)) % 8));
}
than, I write a fuction: byte[] TxSignalPack(double value, Signal signal, ushort messageLength), FYI. I did a quick test and obtained my expected values, but I can't guarantee that it is free of issues.
/// <summary>
/// Function to pack a signal into a CAN data message
/// </summary>
/// <param name="value">Value to be packed</param>
/// <param name="signal">Signal containing dbc information</param>
/// <param name="messageLength">Message byte number</param>
/// <returns>Returns a unsigned data message which maybe larger than 8 bytes</returns>
public static byte[] TxSignalPack(double value, Signal signal, ushort messageLength)
{
int64_T iVal;
uint64_T bitMask = signal.BitMask();
// Apply scaling
var rawValue = (value - signal.Offset) / signal.Factor;
if (signal.ValueType == DbcValueType.IEEEFloat)
iVal = (long)FloatConverter.AsInteger((float)rawValue);
else if (signal.ValueType == DbcValueType.IEEEDouble)
iVal = DoubleConverter.AsInteger(rawValue);
else
iVal = (int64_T)Math.Round(rawValue);
// Apply overflow protection
if (signal.ValueType == DbcValueType.Signed)
iVal = CLAMP(iVal, -(int64_T)(bitMask >> 1) - 1, (int64_T)(bitMask >> 1));
else if (signal.ValueType == DbcValueType.Unsigned)
iVal = CLAMP(iVal, 0L, (int64_T)bitMask);
// Manage sign bit (if signed)
if (signal.ValueType == DbcValueType.Signed && iVal < 0)
{
iVal += (int64_T)(1UL << signal.Length);
}
// Pack signal
var txMessageBitArray = new BitArray(messageLength * 8);
byte[] txMessgeByteArray = new byte[messageLength];
if (signal.Intel()) // Little endian (Intel)
{
txMessageBitArray = OrBitArrays(txMessageBitArray, new BitArray(BitConverter.GetBytes((uint64_T)iVal & bitMask)));
// now txMessageBitArray length >=64
txMessageBitArray.LeftShift(signal.StartBit);
//txMessageBitArray and txMessgeByteArray can be different, so I use my function
BitArrayCopyToByteArray(txMessageBitArray, txMessgeByteArray);
return txMessgeByteArray;
}
else // Big endian (Motorola)
{
txMessageBitArray = OrBitArrays(txMessageBitArray, new BitArray(BitConverter.GetBytes((uint64_T)iVal & bitMask)));
// now txMessageBitArray length >=64
txMessageBitArray.LeftShift(GetStartBitLE(signal, messageLength));
//txMessageBitArray and txMessgeByteArray can be different, so I use my function
BitArrayCopyToByteArray(txMessageBitArray, txMessgeByteArray);
return txMessgeByteArray.Reverse().ToArray();
}
}
/// <summary>
/// Or two different length BitArray
/// </summary>
/// <param name="bitArray1"></param>
/// <param name="bitArray2"></param>
/// <returns></returns>
public static BitArray OrBitArrays(BitArray bitArray1, BitArray bitArray2)
{
int maxLength = Math.Max(bitArray1.Length, bitArray2.Length);
BitArray extendedArray1 = new BitArray(maxLength);
BitArray extendedArray2 = new BitArray(maxLength);
for (int i = 0; i < bitArray1.Length; i++)
{
extendedArray1[i] = bitArray1[i];
}
for (int i = 0; i < bitArray2.Length; i++)
{
extendedArray2[i] = bitArray2[i];
}
return extendedArray1.Or(extendedArray2);
}
/// <summary>
/// Function BitArray CopyTo ByteArray when ByteArray Length smoller than BitArray
/// </summary>
/// <param name="bitArray"></param>
/// <param name="byteArray"></param>
public static void BitArrayCopyToByteArray(BitArray bitArray, byte[] byteArray)
{
if (byteArray.Length*8 >= bitArray.Length)
{
bitArray.CopyTo(byteArray, 0);
}
else
{
BitArray bitArray1 = new BitArray(byteArray.Length*8);
for (int i = 0; i < bitArray1.Length; i++)
{
bitArray1[i] = bitArray[i];
}
bitArray1.CopyTo(byteArray, 0);
}
}
@lihaiyonggood looks good but i can say that working with bitarray is really slow. I allready modified the packer class in a branch i created and it works. If you want u can use that or apply suggestions to that. https://github.com/Uight/DbcParser/tree/Functions-for-unpacking-based-on-byte-arrays
For my case i needed high performance as we read around 500 messages per second and if decoding takes to long we build up a buffer which i would rather avoid. If you write a new method you should probably check the performance (optimally using the benchmark nuget or atleast do some measurements with stopwatches). For me i found that BitArray is around 10 times slower than using the way i did it directly on byte arrays
im just waiting for a reponse from @EFeru because of the Pack/Unpack state function because i dont get why they are needed and i would like to clear up the interface for the packer class. One more thing is that i would like using ref in the class. i also would like to have a limiter function based on signal in the packer which is also unclear atm.
Suggestion: instead of reinventing the wheel, try https://github.com/barncastle/BitsKit
@Uight sorry for this late reply. Regarding your question, I am not sure what do you mean by "Pack/Unpack state function"? These are just functions to encode and decode meaninfull physical data back and forth into CAN messages.
On the other hand, I am also thinking how the functions should be updated for more flexibilty. My proposal is to update the Model for Message and Signal as follows:
public class Message
{
...
public byte[] data;
}
public class Signal
{
...
public double value;
}
then the Pack/Unpack methods will become something like this:
private void RxMessageUnpack(Message message)
private void TxMessagePack(Message message)
The user needs to popuplate the data
array before Unpacking and then collect the value
from the Signals list. For Packing the oppsite procedure needs to be done, write value
in all Signals, Pack, and get data
array. This is something in the lines @lihaiyonggood suggested and I think also done by cantools.
What do you guys think about this approach?
@EFeru regarding the TxStatePack vs TxSignalPack or RxStateUnpack vs RxSignalUnpack: The "State" method have no UnitTests while the signal methods do; The "State" methods do not apply scaling or offset which as far as i understand it is just wrong as all signals must be scaled and offset by the values specified; The "State" methods can only handle unsigned integer values (propably therefor the name). The only possible value for which this makes a difference to the "Signal" methods is when the value is longer than around 50bits in which case the Signal method which works with double loses some precision in the value. But if that is the reason for this method then a method for long (signed 64bit) should exist too. Generally speaking i think the "State" methods are not really usefull compared to the "Signal" methods as the only case they would be usefull is super rare and then not tested and not even usefull for signed which is why i would like to know what these were intended for initially.
regarding the interface with the signal value and signals in messages and then message.Pack you suggested i think this might cause some problems. As far as i know the CanTools and most other tools use a interface close to this: message.Pack(Dictionary<string,double> signalValues) This is a more loose implementation but it works around the problem of the default values. E.g. in your solution if a signal does not have a value set the value is 0. If you apply scaling and offset to that that might change the value and in the end you end up with a value you dont want. In the more loose implementation the value just dont gets set which leaves all bits at 0 for sure. Personally i have seen many cases where this all bits are zero case is used to detect if a signal was written yet which is why i would prefer this over having the values in the signals at least for packing. For unpacking it doesnt really matter that much i would just use something like:
public Dictionary<string,double> UnpackMessage(byte[data], Message message) public byte[] PackMessage(Message message, Dictionary<string,double> values)
or use it as a extension of message;
Also this implementation would keep out mutable data out of the message and signal classes if its still the aim to make them immutable.
@Uight I tested the code on this branch: https://github.com/Uight/DbcParser/tree/Functions-for-unpacking-based-on-byte-arrays, The function “private static byte GetStartBitLE(Signal signal, int messageByteCount = 8)“ needs to be changed to“” private static UInt16 GetStartBitLE(Signal signal, int messageByteCount = 8)“. Otherwise, there will be a problem when the bit exceeds 255. please check it
@Uight , let me explain the background for the TxStatePack and RxStateUnpack methods. Actually, all the Pack and Unpack methods, I am using them in embedded systems (of course converted to c code) and that is where they originate from. This is why you see the data types redefinitions uint8_T, int64_T, etc.
, since they are almost a direct copy to/from c code. The TxStatePack, RxStateUnpack are there for computational efficiency reasons. When having many booleans (1 bit or byte), other bytes or fixed types, it is more efficient to avoid any extra un-needed calculations (like multiplications, divisions, additions) especially in floating point precision.
Now, for C# we might still want efficiency depending on the situation. Personally, I would still keep them in since they are just a simpler version of the TxSignalPack and RxSignalUnpack by avoiding scale and offset.
I agree on packing signals, that signals that are not set should be 0 in the packed message. I didn't look into details of your implementation, but if you have something working please feel free to create a PR.
@EFeru ill probably look into this at the weekend more and most likely create a PR. in your embedded code how do you switch between calling the state method and the Signal method? I see theres a IsInteger property which could maybe be used but that only goes on the factor being a whole number. I could see using a property in signal to store if factor is 1 and offset is 0 so that the scaling is not applied but it would still need an if for that but you would have to have the same if somewhere to. The relevant check if you could use the State method would be that dbcValueType is unsigned and the factor is 1 and the offset is 0 wouldnt it be?
In the embedded code, I only use the Packer functions (no dbc parser because is really low level c code), so I do the dbc parsing manually so to say. Se bellow the an example for a message in c code.
uint64_T TxSignalPack(float value, uint8_T startBit, uint8_T length, uint8_T isSigned, float factor, float offset)
uint64_T TxStatePack(uint64_T value, uint8_T startBit, uint8_T length)
and then
TxMsg |= TxSignalPack(signal1, 0, 14, TRUE, 0.01, 0);
TxMsg |= TxStatePack(signal2, 16, 13);
TxMsg |= TxSignalPack(signal3, 32, 8, TRUE, 0.5, 0);
TxMsg |= TxSignalPack(signal4, 40, 8, TRUE, 0.5, 0);
TxMsg |= TxStatePack(signal5, 48, 8);
Regarding C#, your question is valid, to do the same we need to distiguish states from floating signals, so there needs to be an if statement. We could call the TxStatePack inside the TxSignalPack if the signal IsInteger and factor is 1 and offset is 0.
@lihaiyonggood, yourt right for message longer than 32 byte you need it to be u16. i corrected that.
@EFeru i added a benchmark to the project to compare the different methods of packing / unpacking and your right there are some bigger differences between the methods that would probably be noticable on an embedded system.
For packing:
BenchmarkDotNet v0.14.0, Windows 11 (10.0.22631.3880/23H2/2023Update/SunValley3) AMD Ryzen 9 5900X, 1 CPU, 24 logical and 12 physical cores .NET SDK 8.0.200 [Host] : .NET 6.0.32 (6.0.3224.31407), X64 RyuJIT AVX2 DefaultJob : .NET 6.0.32 (6.0.3224.31407), X64 RyuJIT AVX2
Method | Mean | Error | StdDev |
---|---|---|---|
Pack_8Byte_BigEndian_Uint64 | 40.5778 ns | 0.3100 ns | 0.2420 ns |
Pack_8Byte_BigEndian_ByteArray | 126.9214 ns | 2.5612 ns | 2.3958 ns |
Pack_8Byte_LittleEndian_Uint64 | 12.1883 ns | 0.1173 ns | 0.0980 ns |
Pack_8Byte_LittleEndian_ByteArray | 100.1948 ns | 0.6864 ns | 0.5359 ns |
Pack_1Signal_Unsigned_NoScale_StatePack | 0.8825 ns | 0.0081 ns | 0.0068 ns |
Pack_1Signal_Unsigned_NoScale_SignalPack | 3.0999 ns | 0.0711 ns | 0.0665 ns |
Pack_1Signal_Unsigned_NoScale_SignalPackByteArray | 49.3012 ns | 0.8965 ns | 0.8386 ns |
For unpacking:
BenchmarkDotNet v0.14.0, Windows 11 (10.0.22631.3880/23H2/2023Update/SunValley3) AMD Ryzen 9 5900X, 1 CPU, 24 logical and 12 physical cores .NET SDK 8.0.200 [Host] : .NET 6.0.32 (6.0.3224.31407), X64 RyuJIT AVX2 DefaultJob : .NET 6.0.32 (6.0.3224.31407), X64 RyuJIT AVX2
Method | Mean | Error | StdDev |
---|---|---|---|
Unpack_8Byte_BigEndian_Uint64 | 107.287 ns | 2.1000 ns | 2.0625 ns |
Unpack_8Byte_BigEndian_ByteArray | 201.915 ns | 4.0086 ns | 4.2892 ns |
Unpack_8Byte_LittleEndian_Uint64 | 81.062 ns | 0.2072 ns | 0.1617 ns |
Unpack_8Byte_LittleEndian_ByteArray | 171.328 ns | 3.2070 ns | 3.1497 ns |
Unpack_1Signal_Unsigned_NoScale_State | 1.077 ns | 0.0037 ns | 0.0029 ns |
Unpack_1Signal_Unsigned_NoScale_Signal | 20.269 ns | 0.1389 ns | 0.1232 ns |
Unpack_1Signal_Unsigned_NoScale_SignalByteArray | 62.919 ns | 0.1156 ns | 0.0902 ns |
i did run the test multiple times but its pretty stable in the times.
right now i finished the byte pack methods completly but not the message packing stuff. i would need to verfiy two testcases though:
One is this:
[Test]
public void BytePackingBigEndianBigger8Byte()
{
var messageLength = 24;
var sig = new Signal
{
Length = 12,
StartBit = 138,
ValueType = DbcValueType.Unsigned,
ByteOrder = 0,
Factor = 1,
Offset = 0
};
var byteMsg = new byte[messageLength];
Packer.TxSignalPack(byteMsg, 4095, sig);
Assert.AreEqual(byteMsg, new byte[] { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 7, 255, 128, 0, 0, 0, 0 });
var valbyte = Packer.RxSignalUnpack(byteMsg, sig);
Assert.AreEqual(4095, valbyte);
}
public void BytePackingBigEndianSmaller8Byte()
{
var messageLength = 5;
var sig = new Signal
{
Length = 12,
StartBit = 13,
ValueType = DbcValueType.Unsigned,
ByteOrder = 0,
Factor = 1,
Offset = 0
};
var byteMsg = new byte[messageLength];
Packer.TxSignalPack(byteMsg, 4095, sig);
Assert.AreEqual(byteMsg, new byte[] { 0, 63, 252, 0, 0 });
var valbyte = Packer.RxSignalUnpack(byteMsg, sig);
Assert.AreEqual(4095, valbyte);
}
Could someone verify the byte array. are the correct bits set?
Wow, cool results and very insightful. Unfortunately I'm off one week. I'm not sure if @Whitehouse112 would want to check...
@EFeru I’d like but unfortunately I’m off for two week too. I can check by the end of August
Okay then i now validated the stuff with a fake dbc file:
and then this code.
both messages as encoded exactly the same so i suppose my code is right.
@EFeru and @Whitehouse112 i created a pull request for my current implementation which is could be a base to build on function like message build etc. All changes should be compatible with old builds. all unit test running and more unittest added for the new byte pack/unpack functions.
I also added a Benchmark project to the solution. Feel free to remove that if you think it shouldnt be part of the solution.
i need ref because i dont want to reassigne byte ararys all the time.
I would avoid the ref keywork if not absolutely required.
A
@Adhara3 ref was removed in the final pull request for this
If the dbc file contains CAN-FD messages that can be longer than 64 bit the unpack functions are not usable. Also i think most can implementations work by giving you a byte array for the received data anyway (peak, innodisk, esd to name a few).
Therefor i adjusted the unpack signals method like this:
And then adjust the code from before to
this seems to work in a manual test. i tried to reuse most of the code. Probably is also not the fasted solution but should work.
~Problem with this is that the BitArray.ShiftRight() is not available in all targeted frameworks. For .net461 there is no solution for .netstandard2.0 you could upgrade the solution to .netstandard2.1.~