Building an Encoder

[Kyle Kelly]

I am attempting to build an encoder to translate values (integer values of arbitrary length) input by the user at the command line to 64-bit integer values written to the CAN. The documentation shows that 

uint64_t ourRoundingWriteEncoder(CanSignal* signal, CanSignal* signals, int signalCount, double value, bool* send) 

should be used as a template for the function definition. Based on this, I have a config.json file 

{ "buses":{

"hs": {

"controller": 1,

"raw_writable": true,

"speed": 500000

}

},

"messages": {

"0x201": {

"bus": "hs",

"bit_numbering_inverted": false,

"signals": {

"My_Name": {

"generic_name": "my_signal",

"bit_position": 0,

"bit_size": 64,

"encoder": "ourWriteEncoder",

"writable": true

}

}

},

"extra_sources": [

"my_handlers.cpp"

]

}

Here I have chosen the signal to to be 64-bits wide because I want to write the entire 64-bits to the CAN (the input from the user will be nice from 1 to 4 decimal values long). Then in my_handler.cpp I have 

void ourWriteEncoder(CanSignal* signal, CanSignal* signals,

int signalCount, double value, bool* send) {

// Look up the numeric signals we need to send and abort if missing

CanSignal* mySignal = lookupSignal("my_signal", signals, signalCount);

if(rpmSignal == NULL) {

debug("Unable to find signal, can't send signal");

return;

}

uint64_t can_value = 0;

if(value < 5000) {

can_value = 0x4E00000027100000;

}

openxc::can::write::sendEncodedSignal(rpmSignal, can_value, true);

return;

}

When I try to compile I get the following error:

error: invalid conversion from 'void (*)(CanSignal*, CanSignal*, int, double, bool*)' to 'SignalEncoder {aka long long unsigned int (*)(CanSignal*, _openxc_DynamicField*, bool*)}' [-fpermissive]

 };

 ^

I seem to be passing all the right arguments to sendEncodedSignal but I can't trace back (in canwrite.cpp and canutil.cpp) where SignalEncoder is being called from. I thought that I was essentially doing the encoding myself by passing the 64-bit int "can_value". Any insights would be appreciated.

Thanks,

Kyle

[Zac]

Hi Kyle,

In your particular case, your encoder needs to return a uint64_t value. Once your encoder does it's function and returns the uint64_t, then another function elsewhere will call the "sendSignal" function. Take a look at this example: https://github.com/openxc/vi-firmware/blob/0ee0b5ab03174a8729ca417d101848584b26b993/src/can/canwrite.cpp#L23

Does that make sense?

In your particular case, the function "encodeAndSendSignal" is called. Part of that function is to call your specific encoder and then send the returned value: https://github.com/openxc/vi-firmware/blob/0ee0b5ab03174a8729ca417d101848584b26b993/src/can/canwrite.cpp#L23.

- Zac

[Kyle Kelly]

Zac,

Thanks for the reply. I didn't realize I had to return a uint64_t value. I saw some other handlers in the documentation return void so I thought it could be user defined as long as I sent the signal. I must be missing something in the flow of the code... Where exactly is "encodeAndSendSignal" called? Is that called by default when you include "encoder" in the .json file?

I have changed my_handler.cpp to this:

uint64_t ourWriteEncoder(CanSignal* signal, CanSignal* signals,

int signalCount, double value, bool* send) {

// Look up the numeric signals we need to send and abort if missing

CanSignal* mySignal = lookupSignal("my_signal", signals, signalCount);

if(mySignal == NULL) {

debug("Unable to find signal, can't send signal");

return;

}

uint64_t can_value = 0;

can_value = static_cast<uint64_t>(value) / 64; //create 64-bit value and perform operation

can_value <<= 56; //shift to CAN bytes 0 and 1

can_value = can_value | 655360000; // OR with 0x27100000

return can_value;

}

And get the following error:

signals.cpp: In function 'uint64_t ourRPMWriteEncoder(CanSignal*, CanSignal*, int, double, bool*)':

signals.cpp:42:4: error: return-statement with no value, in function returning 'uint64_t {aka long long unsigned int}' [-fpermissive]

    return;

    ^

signals.cpp: At global scope:

signals.cpp:104:1: error: invalid conversion from 'uint64_t (*)(CanSignal*, CanSignal*, int, double, bool*) {aka long long unsigned int (*)(CanSignal*, CanSignal*, int, double, bool*)}' to 'SignalEncoder {aka long long unsigned int (*)(CanSignal*, _openxc_DynamicField*, bool*)}' [-fpermissive]

 };

 ^

I realize we're getting a bit deeper into the code and I'm not asking you to debug for me but I'm confused by the second error. It seems that SignalEncoder is expecting a type openxc_DynamicField (isn't that any number, string or boolean?) but I'm passing a uint64_t. Also, both of your links direct me to the same line of code, I'm not sure if that was intentional.

Thanks,

Kyle

[Zac]

In your updated function, the issue is in your if statement here:

if(mySignal == NULL) {

debug("Unable to find signal, can't send signal");

return;

}

In the event that your mySignal is NULL, then you will return a void. You need to return something that's a uint64_t even if it's empty. Hence the "return statement with no value"

I'm not sure about the second error exactly. Can you try the above fix, and then see if that fixes the second issue as well? If that doesn't fix both, then can you copy that line on your signals.cpp file so we can see what's going on?

Thanks,
Zac

[Zac]

Sorry about the duplicate links. Here's the one I was trying to send: https://github.com/openxc/vi-firmware/blob/0ee0b5ab03174a8729ca417d101848584b26b993/src/can/canwrite.cpp#L83

You may have identified a bug in vi-firmware regarding custom encoders... But let's first try the fix below to see if that helps.

[Kyle Kelly]

Haha I can't believe I missed that return call, thanks for setting me straight there. After the fix, the error is still present:

signals.cpp:106:1: error: invalid conversion from 'uint64_t (*)(CanSignal*, CanSignal*, int, double, bool*) {aka long long unsigned int (*)(CanSignal*, CanSignal*, int, double, bool*)}' to 'SignalEncoder {aka long long unsigned int (*)(CanSignal*, _openxc_DynamicField*, bool*)}' [-fpermissive]

 };

 ^

Here are lines 100-106 from signals.cpp:

const int MAX_SIGNAL_COUNT = 2;

CanSignal SIGNALS[][MAX_SIGNAL_COUNT] = {

    { // message set: generic

        {message: &CAN_MESSAGES[0][0], genericName: "my_signal", bitPosition: 0, bitSize: 64, factor: 1.000000, offset: 0.000000, minValue: 0.000000, maxValue: 0.000000, frequencyClock: {0.000000}, sendSame: true, forceSendChanged: false, states: NULL, stateCount: 0, writable: true, decoder: NULL, encoder: ourWriteEncoder}, // My_SIGNAL

        {message: &CAN_MESSAGES[0][1], genericName: "left_blinker", bitPosition: 0, bitSize: 8, factor: 1.000000, offset: 0.000000, minValue: 0.000000, maxValue: 0.000000, frequencyClock: {0.000000}, sendSame: true, forceSendChanged: false, states: NULL, stateCount: 0, writable: true, decoder: NULL, encoder: NULL}, // My_Blinker

    },

};

And then lines 107-113:

void openxc::signals::initialize(openxc::diagnostics::DiagnosticsManager* diagnosticsManager) {

    switch(getConfiguration()->messageSetIndex) {

    case 0: // message set: generic

        break;

    }

}

Thanks for all of your help. Let me know if you come up with anything.

Kyle

[Zac]

Ah, so the signalEncoder type has a few different inputs. It should be (CanSignal*, _openxc_DynamicField*, bool*) as mentioned in the error log. The documentation needs to be updated to reflect this change.

Here's the canutil.h header file link where the SignalEncoder typedef if defined: https://github.com/openxc/vi-firmware/blob/master/src/can/canutil.h#L53

Then in order to get the "value" that you send to the encoder, you would do something this:

uint64_t ourWriteEncoder(CanSignal* signal, openxc_DynamicField* value, bool* send) {

      uint64_t can_value = value->numeric_value;

.....

Does that compile properly?

- Zac

[Kyle Kelly]

It does compile, thanks for the help!

I've now run into a new issue. When I return a uint64_t value in my encoder I want to write that entire value to the CAN. However, it only takes the last 32 bits of the value I return (ie if I return 0x4E00000028 it will only take 0x00000028). I have tried sending decimal values with the same result.

I looked into the code and I believe the error results from this line and its corresponding definition in the header file: https://github.com/openxc/vi-firmware/blob/0ee0b5ab03174a8729ca417d101848584b26b993/src/can/canwrite.cpp#L12

I think value should be uint64_t not int. This function then calls bitfield_encode_float and float_to_fixed_point (in write.h and write.c) and defines value as a float, but I think it needs to be a double.

I tried the fix myself and did not get the result I expected, so I might be on the right track but I don't think this is the complete solution. I am also not sure I am editing the correct write.h/.c files because they appear several times (although I think most are deprecated versions). 

I'll continue to look into it but I just wanted to give you an update and see if you had any insight.

Kyle

[Zac]

Hi Kyle,

Looks like you may have found a bug! I created an issue on the vi-firmware github page: https://github.com/openxc/vi-firmware/issues/364.

If you are able to nail down a solution, please go ahead and update the issue and generate a pull request. 

The bitfield files you would need to change are in: /vi-firmware/src/libs/bitfield-c/canutil. The bitfield-c library is a git submodule. The repo can be found here by itself if you want to search through it online: https://github.com/openxc/bitfield-c

[Kyle Kelly]

Zac it looks like this has been resolved, I must must have been editing the incorrect write.h/.c file before. I'll generate the pull request later today.

There is another function in the same write.h/.c file, "eightbyte_encode_float", where intuition says that value should also a double instead of a float. However, since I'm not using this function (yet) I won't touch it.

Thanks,

Kyle

[Kyle Kelly]

Is there a way to create multiple named signal writes to the same arbitration ID? For example, if I have a config.json file like the following:

...

"messages": {

"0x201": {

"bus": "hs",

"bit_numbering_inverted": false,

"signals": {

"FirstSignal": {

"generic_name": "set_first",

"bit_position": 0,

"bit_size": 8,

"writable": true

}

}

},

"0x201":{

"bus": "hs",

"bit_numbering_inverted": false,

"signals": {

"SecondSignal": {

"generic_name": "set_second",

"bit_position": 0,

"bit_size": 16,

"writable": true

}

}

}

...

and then push this to signals.cpp, only the second signal will be generated. It appears as though it overwrites the first signal. This is true even when the bits do not overlap. I tried to manually insert the second signal but had no luck (I may still investigate this route further).  Is there a workaround for this that you're aware of?

Kyle

[Kyle Kelly]

For anyone wondering, this can be done by doing something like the following:

...

"messages": {

"0x201": {

"bus": "hs",

"bit_numbering_inverted": false,

"signals": {

"FirstSignal": {

"generic_name": "set_first",

"bit_position": 0,

"bit_size": 8,

"writable": true

},

"SecondSignal": {

"generic_name": "set_second",

"bit_position": 0,

"bit_size": 16,

"writable": true

}

}

},

...

It won't matter if the bits overlap.