Implementation of Photon CAN bus Driver

[bspranger]

Please consider using this CAN bus driver for implementing CAN on the Photon. Please send me messages on the forum if you have any questions.

[m-mcgowan]

I'm guessing this didn't build for you? We need this for the multithreading build, so please re-insert this line.

[m-mcgowan]

Thank you so much for submitting this PR this is a great effort! 👍

I'm wondering about the public API in wiring. Can we simplify it so that users don't need to use CAN_Message_Struct instances?

e.g.

can1.write(id, "abcdefgh");

uint8_t buf[8];
uint16_t id;
can1.read(id, buf);

I don't know the CAN protocol - are the other fields in the CAN_Message_Struct significant to the user, or is ID and Message all they need?

[monkbroc]

I'll review and test this PR. I used to work with CAN in a previous job.

[monkbroc]

@bspranger I merged all your commits into one, then put my changes on top.

I kept the same idea to have a queue of received and transmitted messages, but I moved the implementation to C++.

It now compiles and runs correctly with the latest firmware on the Photon and Electron, with the modular firmware (dynalib) and without (single binary).

The Electron has 2 CAN channels available and the driver works with either. The Photon has only 1 CAN channel exposed. The Core can't use CAN because that peripheral shares memory with the USB.

I added functionality for filtering based on id/mask (14 filters per channel).

I added unit tests for the fixed-sized queue and integration tests for each CAN channel.

Here's the user-facing API:

CANChannel can(CAN_D1_D2);

void setup() {
    can.begin(125000);
    // accept one message. If no filter added by user then accept all messages
    can.addFilter(0x100, 0x7FF);
}

void loop() {
    CANMessage Message;

    Message.id = 0x100;
    can.transmit(Message);

    delay(10);

    if(can.receive(Message)) {
        // message received
    }
}

So far I did all my tests with the CAN in loop-back mode. I'm going to try with real hardware tonight.

One comment about my implementation is that having a HAL driver be a C++ class makes the code convoluted since the HAL_CAN_xyz functions just delegate to the C++ object. Maybe it's better to just keep the implementation of the CAN driver in can_hal.cpp in C++ functions and put the state in a separate struct.

[bspranger]

@monkbroc the can.addFilter call probably needs a parameter to specify if the filter is standard or extended CAN frames.

[monkbroc]

addFilter call probably needs a parameter to specify if the filter is standard or extended CAN frames.

It's there. It defaults to standard, that's all.

bool addFilter(uint32_t id, uint32_t mask, HAL_CAN_Filters type = CAN_FILTER_STANDARD);

I'll link a pull request to the Particle documentation repository with all these details when we agree on the user-facing interface.

[m-mcgowan]

Outstanding job @monkbroc! 👍 I've skim-read the code - all looks good to me.

@bspranger Thanks again for taking the initiative to start work on a CAN driver - much appreciated! Please try out this PR and let us know if it meets your needs.

Aside from documentation, is there any more work remaining here? Would be great to include this in the 0.4.9 release next week!

[monkbroc]

It's all good from my side. My main concern was the style of having HAL functions delegate to an implementation object. I'd like to know if you have a suggestion in that area @m-mcgowan.

[m-mcgowan]

It's fine - we do this in other layers - map a C functional interface to a C++ object. Some small things to add: add attribute((packed)) on the CANMessage class, and also add a uint16_t size field so that we have some way to extend the structure safely in future in a backwards compatible way.

[monkbroc]

add attribute((packed)) on the CANMessage

I made sure to order the members inside CANMessage to minimize struct size. There are no padding bytes so __attribute((packed)) isn't necessary. With adding a uint8_t size, CANMessage is now 16 bytes so it will copy efficiently.

[m-mcgowan]

Could someone please rebase this on develop so we can merge, thanks!

[monkbroc]

Done.

[bspranger]

Is this still going to be merged for the next firmware? I don't want to see it forgotten.

[m-mcgowan]

Yes, one of our engineers will be testing this soon, and assuming it checks out, will be merged for 0.4.9. Don't worry, it's not forgotten! CAN bus support is awesome!

[bspranger]

m-mcgowan your fix with the newlib-nano references this pull request. Is this an issue because the rx/tx queue sizes can be passed in by the user? If we just put in a fixed size right now would it resolve the problem? Or was there some other issue you seen?

[m-mcgowan]

Please ignore that - it was a mistake on my part - pasted a message into the wrong tab. I deleted the comment but the issue still retains the link. Sorry for the confusion!

[bspranger]

Woot! I can't wait to go to the Web interface and compile an app using CAN!

[monkbroc]

Great to see this going in the firmware. Thanks for your work @bspranger!

[m-mcgowan]

Just writing up skeleton docs for this. I noticed the message exchange happens via transmit() and receive() functions. Is there a precedence here, or should we make it follow the same naming conventions as other peripherals, e.g. write() and read(). I'm on the fence here - just raising to be sure consistency with existing similar APIs has been considered.

cc: @jenesaisdiq - for reference an example of the API is at - https://community.particle.io/t/photon-can-bus-in-progress/12634/113

[monkbroc]

transmit and receive are the usual terminology for CAN messages.

I would say to keep write and read for single byte APIs like I2C, SPI, serial, EEPROM.

[monkbroc]

Add @monkbroc to the CAN docs PR and I'll review.

[m-mcgowan]

The docs PR is here - particle-iot/docs#269. As you'll see it's bare-bones - I've only taken the example you gave on the forum. Would be awesome of you (or @bspranger) could flesh out the docs.

We are going to release 0.4.9 on Monday so I hope that's sufficient time for you.

[monkbroc]

I added documentation for each method of CANChannel as particle-iot/docs#273

[straccio]

Good job! Really tnx!
It could be possible move i2c bus on Photon to, for example, D2/D3 or better to D7/D8 ?

Best regards

[technobly]

@straccio I'm guessing you meant CAN instead of I2C. CAN is only available on D1,D2 on the Photon and D1,D2 and C4,C5 on the Electron. The Electron supports two separate CAN interfaces. Since these connect to a hardware peripheral, they can only be moved to alternate pins for CAN. The only other set of alternate pins that are available are being used for USB data+ and data-.

Just in case you do want to know though, I2C is only available on D0,D1 on Photon/Core and alternative pins C4,C5 on Electron using Wire1 (note this is not a 2nd I2C interface but the same one on alternate pins). There are two other I2C peripherals on the STM32F205, however one is use internally on the Electron for the PMIC and Fuel Gauge and the other is on pins used as serial (USART) communication to the Electron's cellular modem.

[straccio]

If i need use i2C and CAN in a PHOTON i need to use software emulated i2C?

[technobly]

Yes that could definitely be a solution. I haven't seen a Software I2C library for the Photon yet though, so if you find/adapt one and it works well please post it on the community and/or submit a library :)

[straccio]

Or i can edit the HALl and use the pins used as serial (USART)?

[technobly]

You could, but you'd have to do some serious hacking on an Electron. If you still care to use the modem, you'd have to wire another serial interface to the modem and it wouldn't be hardware flow control capable, and firmware modification would be up to you.

As painful as it may sound when there is CAN hardware on the microcontroller you're using, another alternative is to use a SPI based CAN transceiver like the one used here (I've used these before and they work well). I2C based CAN transceivers must exist as well.. I don't have a recommendation for one though.
https://www.sparkfun.com/products/13262