Hi,

I'm trying to add CAN transmission to MySensors.

This is my fork: https://github.com/AdamSlowik/MySensors/tree/feature/can

This is how I connected my CAN modules to arduino (except pin INT of CAN module is connected to pin 2 of arduino):
https://www.14core.com/wiring-the-two-mcp2515-stand-alone-can-controller-with-spi/

My CAN module is popular one (mpc2515 with tja1050)

My fork contains following examples:
-GatewaySerialCAN
-CANSwitch (simpler version of BinarySwitchSleepSensor)

Heavy lifting (MCP2515 driver stuff) is done by https://github.com/coryjfowler/MCP_CAN_lib

After uploading mentioned sketches to arduinos, sensor sends frame. I belive this is request for new node ID (it's 7 bytes long).
Gateway recives this frame (data is same as in sensor side). After that strange things happens. Gateway ends up in some endless loop calling 'transportDataAvailable'. This might be normal but 'IF' statement 'if(!hwDigitalRead(CAN0_INT))' is always true. Because this statement is true program reads data from CAN. Since 'IF' statement (in my opinion) is false positiv, data from CAN are garbage (0xFFFFFFFF).

Two easiest explenation for me are:
a) Gateway uses pin 2 and this interfere with communication code
b) Used CAN library is somehow incompatible with MySensors

I belive wiring issue is not the case. All things are soldered in universal board and tested with examples provided by CAN library.

Can someone look at My code especialy at how I use pin 2?
Perhaps someone have CAN modules to reproduce My issue?

Hi @JeeLet, to test my implementation You should:

1. Set up hardware as described in my first post in this thrread.
2. Download source from my fork (https://github.com/AdamSlowik/MySensors/archive/refs/heads/development.zip)
3. Somewhere in Your lib folder swap official MySensors files with those downloaded.
4. Upload "CANSwitch.ino" to first arduino and "GatewaySerialCAN.ino" to second one. Both files can be found in examples.

If everything is fine then You Should get results similar to those presented in my post dated to "14 Jul 2020, 20:42"

Please let Us know if You managed to reproduce My results.

I found a bug in my code. Please re-download new version.

If something won't work please enable CAN debug by adding:
#define MY_DEBUG_VERBOSE_CAN

add this to gateway and to node sketch

I'm not sure if We understood each other. This line #define CAN_CLOCK MCP_16MHZ should be in Your sketch. You should not modify MyConfig.h.
Libraries (in general not only MySensors) should not be modified. MyConfig.h is part of library.

Let's take a look at this code from MyConfig.h

#ifndef CAN_CLOCK
#define CAN_CLOCK MCP_8MHZ
#endif. 

This is if statement. If user didn't defined CAN_CLOCK define it as CAN_CLOCK MCP_8MHZ
If user defined (in sketch) CAN_CLOCK then do nothing.

So You can treat MyConfig.h as a set of defaults which can be overridden from sketch.

I have put all relevant (in My opinion) properties from MCP_CAN_lib into MyConfig.h, so You can override them. If I have missed some relevant property please let Me know.

@virtualmkr said in CAN bus transport implementation:

I'm not a "radio guy", but I think this code change looks fine. When sanity check fails then MySensors core calls transportInit() which would try to re-initialise the MCP2515 module.

Ok. I'll update code.

@virtualmkr said in CAN bus transport implementation:

I have done some modifications in the CAN transport files.
@Adam-Slowik I would like to create a PR in your forked repo. Then we can discuss the code changes in GitHub. Would that be Ok for you - do you have time for this?

Sure. Make PR. I'll try to review it within 48h.

@virtualmkr said in CAN bus transport implementation:

When I send 32 byte message I observed the sub telegrams do not always arrive in send order

Other limitation I see is the limited CAN telegram buffer of 3 packets in the MCP2515 module. I'm afraid for reliable operation with MySensors a change from polling to interrupt mode is required.

I think this two issues are linked. Now packets are sent in correct order (could You verify it?). If packets are sent in short periods We should use interrupt to keep MCP2515 buffers empty. When We fail to keep buffers empty (due to polling mechanism) We receive sub telegrams in mixed order. This theory could be true if MCP2515 buffers are implemented as LIFO. I'll look into documentation to figure it out.
Anyway both fixes are worth to implement. I'll try to do this over weekend.

Are You still have an issue when MY_DEBUG is deactivated? Have You tried to add delay as is was suggested in one of the threads You found?

So library gives us 3 interesting functions:

INT8U checkError(void);           
INT8U errorCountRX(void);                                           
INT8U errorCountTX(void);   

Full description can be found in library source code and in documentation chapter 6:
https://ww1.microchip.com/downloads/en/DeviceDoc/MCP2515-Stand-Alone-CAN-Controller-with-SPI-20001801J.pdf

long story short:

1. INT8U checkError(void); returns:
   -CAN_CTRLERROR when things are bad or very bad
   -CAN_OK when things are ok or not so bad (error counters below 96)
   I would like to chage implementation of sanity check to something like this:

bool transportSanityCheck(void)
{
	return (CAN0.getError()==CAN_OK)
}

It won't blink diodes in case of communication problems, but will provide some feedback about communication to MySensors library.

2)INT8U errorCountRX(void); return error counter for received messages. AFAIK this counter is incremented when error occurs and decremented when message is received. I'm not a 'radio guy' but i think this could be returned as int16_t transportGetReceivingSNR(void) or int16_t transportGetReceivingRSSI(void). I know SNR and RSSI are radio related parameters, but perhaps We could bend the border here. Maybe some 'radio guy' could say something about this idea?

3)INT8U errorCountTX(void); same situation as with #2. 'Radio guy' opinion would by great.

Again ideas #2 and #3 are not solving problems, but providing feedback to MySensors.

Are My ideas ok, or do they do not pass sanityCheck()

@JeeLet said in CAN bus transport implementation:

The original idea was: how to check the status of the CAN bus.
. is there any dialogue ?
. are there any messages?
of course this can be done with MySensors.
But to get as close as possible to the communication channel (the CAN bus) I thought it was easy to do.

That is something I can work with. I will check if used library provides some function to check bus status.

@JeeLet said in CAN bus transport implementation:

my time available for the computer will soon be reduced (purchase and work of a house)

I totally understand.

@JeeLet said in CAN bus transport implementation:

The list of #define go up for the MySensors sketch .
( if I'm not mistaken ?)

You are right.

@JeeLet said in CAN bus transport implementation:

can be added, for testing and controlling the CAN bus
#define CAN_MSGAVAIL
#define CAN_OK
#define CAN_CTRLERROR

I'm afraid i do not understand what You want to achieve.
To clarify things lets look at CAN_CLOCK

CAN_CLOCK can take values: 0 or 1 or 2 those values are defined in mcp_can_dfs.h:

#define MCP_20MHZ    0
#define MCP_16MHZ    1
#define MCP_8MHZ     2

If You want to use 8MHz You can
#define CAN_CLOCK 2
or
#define CAN_CLOCK MCP_8MHZ
Both lines will do the same. However #define CAN_CLOCK MCP_8MHZ is recommended approach.
So CAN_CLOCK can take one of values values (0,1,2), and user (based on used hardware) should pick one of them.

Next example could be CAN_INT. User should be able to pick which interrupt pin should be used.

Now when it comes to:

#define CAN_MSGAVAIL    
#define CAN_OK  
#define CAN_CTRLERROR  

in mcp_can_dfs.h We can find a section:

#define CAN_OK             (0)
#define CAN_FAILINIT       (1)
#define CAN_FAILTX         (2)
#define CAN_MSGAVAIL       (3)
#define CAN_NOMSG          (4)
#define CAN_CTRLERROR      (5)
#define CAN_GETTXBFTIMEOUT (6)
#define CAN_SENDMSGTIMEOUT (7)
#define CAN_FAIL       (0xff)

This We can treat as return code dictionary.
For example when I initialize CAN in MyTransportCAN.cpp:

	if (CAN0.begin(MCP_STDEXT, CAN_SPEED, CAN_CLOCK) != CAN_OK) {
		canInitialized = false;
		return false;
	}
	canInitialized = true;

I could write if (CAN0.begin(MCP_STDEXT, CAN_SPEED, CAN_CLOCK) != 0 but recommended approach is to use dictionary key (CAN_OK) instead of dictionary value (0)

Now overriding CAN_OK to 777 instead of 0 is possible, but it won't cause nothing but chaos. If We do that the function CAN0.begin(MCP_STDEXT, CAN_SPEED, CAN_CLOCK) would return 777 instead of 0. I don't think user should do that.

Code from mcp_can.cpp:

INT8U MCP_CAN::checkReceive(void)
{
	INT8U res;
	res = mcp2515_readStatus();                                         /* RXnIF in Bit 1 and 0         */
	if (res & MCP_STAT_RXIF_MASK) {
		return CAN_MSGAVAIL;
	} else {
		return CAN_NOMSG;
	}
}

This function returns dictionary key CAN_MSGAVAIL or CAN_NOMSG but if We could stop in debug We would see 3 or 4
Again overriding CAN_MSGAVAIL to let's say 42 won't give user nothing.

@JeeLet said in CAN bus transport implementation:

return of the last 24 hours .
CAN bus no error, good dialogue, no hole in the measurements and commands.
the hardware base on the bus is :
gateway + node 55 + node 56 .
I will add another node 57.
I think to go until 6 nodes

I keep My fingers crossed. Great job.

I'm not sure if We understood each other. This line #define CAN_CLOCK MCP_16MHZ should be in Your sketch. You should not modify MyConfig.h.
Libraries (in general not only MySensors) should not be modified. MyConfig.h is part of library.

Let's take a look at this code from MyConfig.h

#ifndef CAN_CLOCK
#define CAN_CLOCK MCP_8MHZ
#endif. 

This is if statement. If user didn't defined CAN_CLOCK define it as CAN_CLOCK MCP_8MHZ
If user defined (in sketch) CAN_CLOCK then do nothing.

So You can treat MyConfig.h as a set of defaults which can be overridden from sketch.

I have put all relevant (in My opinion) properties from MCP_CAN_lib into MyConfig.h, so You can override them. If I have missed some relevant property please let Me know.

@JeeLet said in CAN bus transport implementation:

I'm going from 8Mhz to 16Mhz (in development/MyConfig.h on line 306)

In My opinion You should not override MyConfig.h. If You want to go for 16MHz You should put:

#define CAN_CLOCK MCP_16MHZ

in Your sketch. Preferably somewhere around line:

#define MY_CAN

@JeeLet said in CAN bus transport implementation:

can you explain how I can bring up in the sketch the parameters that are related to the MCP_CAN_lib library

If it's a parameter that is listed in MyConfig.h simply define it like 'CAN_CLOCK'. If parameter is not listed in MyConfig please let me know exactly what parameter You would like to modify, then I will try to figure it out.

@JeeLet said in CAN bus transport implementation:

I test what now???

I was at similar position some time ago:

@Adam-Slowik said in CAN bus transport implementation:

Any suggestion what to test next?

I have no idea what to test next. I do not use MySensors. I hope this will change in the future, but as of now I'm a newbie.

If You won't get any advice from more experienced users I would suggest to do some long running test to see if it's stable. If it's stable try to use it in some real use cases.

If You find any bugs in transmission layer post logs here. I will do My best to support CAN, because I will need it in a future.

@JeeLet said in CAN bus transport implementation:

what was the problem ???

Previous messages used 7 bytes long payload. Single CAN frame is able to transfer 8 bytes. So all messages was contained in single frame. Example with time, used 17 (!) bytes long payload. In order to transfer 17 bytes there is need to split data to 3 messages (8+8+1). Next in the other side of wire You have to concat it. This mechanism of splitting and concating had a bug. For details check last 3 commits (at the bottom)
https://github.com/mysensors/MySensors/compare/development...AdamSlowik:development

I found a bug in my code. Please re-download new version.

If something won't work please enable CAN debug by adding:
#define MY_DEBUG_VERBOSE_CAN

add this to gateway and to node sketch