Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implementation of Photon CAN bus Driver #790

Merged
merged 2 commits into from Jan 20, 2016

Conversation

@bspranger
Copy link
Contributor

commented Jan 1, 2016

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.

@@ -186,7 +186,7 @@ int os_semaphore_take(os_semaphore_t semaphore, system_tick_t timeout, bool rese
int os_semaphore_give(os_semaphore_t semaphore, bool reserved);

#define _GLIBCXX_HAS_GTHREADS
#include <bits/gthr.h>
//#include <bits/gthr.h>

This comment has been minimized.

Copy link
@m-mcgowan

m-mcgowan Jan 1, 2016

Contributor

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

return HAL_CAN_Is_Enabled(_channel);
}

#ifndef SPARK_WIRING_NO_CAN

This comment has been minimized.

Copy link
@m-mcgowan

m-mcgowan Jan 1, 2016

Contributor

we don't instantiate global objects in the wiring project directly - rather global wiring objects are kept in the wiring_globals project. This is so that these objects aren't instantiated by the system module which uses some classes from wiring (String, Stream, Print.)

}

txmessage.DLC = canMap[channel]->can_tx_buffer->buffer[canMap[channel]->can_tx_buffer->tail].Len;
txmessage.Data[0] = canMap[channel]->can_tx_buffer->buffer[canMap[channel]->can_tx_buffer->tail].Data[0];

This comment has been minimized.

Copy link
@m-mcgowan

m-mcgowan Jan 1, 2016

Contributor

How about using a for loop here to reduce the code size?

@m-mcgowan

This comment has been minimized.

Copy link
Contributor

commented Jan 1, 2016

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

This comment has been minimized.

Copy link
Member

commented Jan 1, 2016

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

@monkbroc monkbroc force-pushed the bspranger:develop branch from 79dc2a9 to e57da66 Jan 11, 2016

@monkbroc

This comment has been minimized.

Copy link
Member

commented Jan 11, 2016

@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.

@monkbroc monkbroc force-pushed the bspranger:develop branch from e57da66 to 354beac Jan 11, 2016

@bspranger

This comment has been minimized.

Copy link
Contributor Author

commented Jan 11, 2016

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

@monkbroc

This comment has been minimized.

Copy link
Member

commented Jan 11, 2016

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

This comment has been minimized.

Copy link
Contributor

commented Jan 12, 2016

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

This comment has been minimized.

Copy link
Member

commented Jan 12, 2016

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

This comment has been minimized.

Copy link
Contributor

commented Jan 12, 2016

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

This comment has been minimized.

Copy link
Member

commented Jan 12, 2016

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

This comment has been minimized.

Copy link
Contributor

commented Jan 16, 2016

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

bspranger and others added 2 commits Oct 2, 2015
Add CAN driver to HAL
* Add CAN support
* Add CAN interrupts
* Add demo application
Move CAN Driver implementation to C++
* Separate message queue to its own class with tests
* Ensures CAN messages are transmitted in order
* Add filters and error state
* Add CAN to dynalib

Hardware compatibility

STM32F2xx has 2 CAN ports:
RX: PB8 (C5), TX: PB9 (C4) -> CAN1 (on Electron only)
RX: PB5 (D2), TX: PB6 (D1) -> CAN2 (Photon, P1, Electron)

Core cannot use CAN since the RAM is shared with the USB peripheral

@monkbroc monkbroc force-pushed the bspranger:develop branch from 68dc49e to 9b6f66b Jan 16, 2016

@monkbroc

This comment has been minimized.

Copy link
Member

commented Jan 16, 2016

Done.

@bspranger

This comment has been minimized.

Copy link
Contributor Author

commented Jan 20, 2016

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

@m-mcgowan

This comment has been minimized.

Copy link
Contributor

commented Jan 20, 2016

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

This comment has been minimized.

Copy link
Contributor Author

commented Jan 20, 2016

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

This comment has been minimized.

Copy link
Contributor

commented Jan 20, 2016

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!

@m-mcgowan m-mcgowan added this to the 0.4.9 milestone Jan 20, 2016

m-mcgowan added a commit that referenced this pull request Jan 20, 2016
Merge pull request #790 from bspranger/develop
Implementation of Photon CAN bus Driver

@m-mcgowan m-mcgowan merged commit 6ee0d8c into particle-iot:develop Jan 20, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bspranger

This comment has been minimized.

Copy link
Contributor Author

commented Jan 20, 2016

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

@monkbroc

This comment has been minimized.

Copy link
Member

commented Jan 21, 2016

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

@m-mcgowan

This comment has been minimized.

Copy link
Contributor

commented Jan 21, 2016

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

This comment has been minimized.

Copy link
Member

commented Jan 21, 2016

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

This comment has been minimized.

Copy link
Member

commented Jan 21, 2016

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

@m-mcgowan

This comment has been minimized.

Copy link
Contributor

commented Jan 22, 2016

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

This comment has been minimized.

Copy link
Member

commented Jan 22, 2016

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

@straccio

This comment has been minimized.

Copy link

commented Jan 25, 2016

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

This comment has been minimized.

Copy link
Member

commented Jan 25, 2016

@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

This comment has been minimized.

Copy link

commented Jan 25, 2016

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

@technobly

This comment has been minimized.

Copy link
Member

commented Jan 25, 2016

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

This comment has been minimized.

Copy link

commented Jan 25, 2016

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

@technobly

This comment has been minimized.

Copy link
Member

commented Jan 25, 2016

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.