ASFv4 Can Bus library fifo issues

Go To Last Post
13 posts / 0 new
Author
Message
#1
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Hello All,

 

I am working with ATSAME54P20A  on SAME54 Xplained board using Atmel Start with atmel studio 7.

 

I would like to share my findings on the CAN bus driver which is included with Atmel start #define DRIVER_VERSION 0x00000001u.

 

1. The driver doesn't acknowledge the fifo read in can_async_read() function. If I am right RXF0A_F0AI needs to be incremented every time after message has been red. So what actually happens every call to can_async_read() gets the first element of the fifo and doesn't clear them causing the fifo to overflow eventually.

 

2. The driver allocates a random ram address for the FIFO. However only 16bit address can be passed to to the register (Bits 15:0 – F0SA[15:0] Rx FIFO 0 Start Address) so if any address above 0x0000FFFF will be allocated for the uint8_t can1_rx_fifo[CONF_CAN1_F0DS * CONF_CAN1_RXF0C_F0S]; this will cause a severe memory leak.

 

I am shocked to find such basic issues with this library. I couldn't find any similiar posts on this forum, does it mean noone has encountered these problems before?

Last Edited: Thu. Jun 21, 2018 - 12:02 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Szary wrote:
doesn't mean noone has encountered this before?
We don't see many CAN users here and we see even fewer CAN+ASF users so I'm not surprised if you are trailblazing. Of course I guess what many folks may do if they hit this kind of problem is actually raise a support ticket with Atmel/Microchip so we'd never see them here anyway (but you would hope that if a fault had been reported to Atmel they would then have fixed/udated ASF)

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Thanks for the response clawson.

 

This is exacly what I was suspecting as well. So I think I will create a support ticket to the Microchip and keep checking for the updates.

Last Edited: Thu. Jun 21, 2018 - 12:05 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Having same problem with the SAME54 Xplained.

 

It kind of work when I set FIFO to 1 and overwrite instead of blocking in the hpl_can_config.h : 

#define CONF_CAN1_RXF0C_F0S 1

#define CONF_CAN1_RXF0C_F0OM 1

Basically disabling FIFO.

 

With FIFO>1 the incoming messages seems to increment correctly the Put Index RXFnS.FnPI, but the Get Index RXFnS.FnPI seems to be stuck and not changing when message is read from FIFO.  

 

Did you manage to work it out?

 

 

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Hi VK2IM,

 

my temporary fix is to add this to hpl_can.c in _can_async_read() dealing with the CAN you have chosen:

 

if (dev->hw == CAN1) 

{

    static volatile uint32_t standard_receive_index = 0;//added to lib

    f = (struct _can_rx_fifo_entry *)(can1_rx_fifo + hri_can_read_RXF0S_F0GI_bf(dev->hw) * CONF_CAN1_F0DS);
    
    hri_can_write_RXF0A_F0AI_bf(dev->hw,standard_receive_index++);//added to lib

}

 

probably you need to clamp standard_receive_index to your fifo size or something, but it seems to be working anyway.

 

I have submitted a ticket to microchip and I am waiting for the response.

 

Have a look on some atmel examples from previous version of ASF which are working fine. I think they are called mCan back there. 

 

Let me know if this helps.

Last Edited: Fri. Jul 6, 2018 - 12:21 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Hi Szary,

 

Thanks for that.  It seems to work fine when there is low traffic.  It does correctly take care of the Get Index, Put Index and Fill Index of FIFO.  But on a busy bus, when the CAN ISR is too slow and FIFO is being used bit deeper, then the interrupt is not called anymore while the get and put index are different and data are still stuck in FIFO.

 

I still think that the get index should be somehow increased automatically.  Maybe by getting data out of fifo differently or setting some flag or something like that.  I do not know.  Other option might be to hang inside ISR in while(fill_index) to get all data out of fifo.

 

Thank again.  Very helpful.  There is a light on the end of the tunnel :)

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Yes it should be increased automatically but this is really poorly written driver which doesn't do it.

 

I found the bit from previous asf which I based my fix on.  I forgot to clamp the index to fifo size:

 

if (status & MCAN_RX_FIFO_0_NEW_MESSAGE) {
        mcan_clear_interrupt_status(&mcan_instance, MCAN_RX_FIFO_0_NEW_MESSAGE);
        mcan_get_rx_fifo_0_element(&mcan_instance, &rx_element_fifo_0,
                standard_receive_index);
        mcan_rx_fifo_acknowledge(&mcan_instance, 0,
                standard_receive_index);
        standard_receive_index++;
        if (standard_receive_index == CONF_MCAN0_RX_FIFO_0_NUM) {
            standard_receive_index = 0;
        }

If  your interrupt is stopped being called your fifo is probably full. You have to make sure you don't overflow your fifo by reading it fast enough. If you are plugging your device into existing bus maybe you need set some filters so only the messages you need are being put on fifo?

 

And the bit from the datasheet about fifo ack:

Bits 5:0 – F0AI[5:0] Rx FIFO 0 Acknowledge Index
After the Host has read a message or a sequence of messages from Rx FIFO 0 it has to write the buffer
index of the last element read from Rx FIFO 0 to F0AI. This will set the Rx FIFO 0 Get Index RXF0S.F0GI
to F0AI + 1 and update the FIFO 0 Fill Level RXF0S.F0FL.

 

39.6.7 FIFO Acknowledge Handling
The Get Indexes of Rx FIFO 0, Rx FIFO 1 and the Tx Event FIFO are controlled by writing to the
corresponding FIFO Acknowledge Index (refer to 39.8.29 RXF0A, 39.8.33 RXF1A and 39.8.47 TXEFA).
Writing to the FIFO Acknowledge Index will set the FIFO Get Index to the FIFO Acknowledge Index plus
one and thereby updates the FIFO Fill Level. There are two use cases:
When only a single element has been read from the FIFO (the one being pointed to by the Get Index),
this Get Index value is written to the FIFO Acknowledge Index.
When a sequence of elements has been read from the FIFO, it is sufficient to write the FIFO
Acknowledge Index only once at the end of that read sequence (value: Index of the last element read), to
update the FIFO’s Get Index.
Due to the fact that the CPU has free access to the CAN’s Message RAM, special care has to be taken
when reading FIFO elements in an arbitrary order (Get Index not considered). This might be useful when
SAMD5x/E5x Family Data Sheet
CAN - Control Area Network
reading a High Priority Message from one of the two Rx FIFOs. In this case the FIFO’s Acknowledge
Index should not be written because this would set the Get Index to a wrong position and also alters the
FIFO’s Fill Level. In this case some of the older FIFO elements would be lost.
Note: The application has to ensure that a valid value is written to the FIFO Acknowledge Index. The
CAN does not check for erroneous values.

© 2018 Microchip Technology Inc. Datasheet DS60001507B-page 1265

Last Edited: Fri. Jul 6, 2018 - 02:12 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Thanks again Szary.  It all makes sense now.  I could not find this section under RX FIFO of datasheet and I didn't go pass or into TX FIFO where this section is.  My bad.

 

I still believe that if two CAN messages arrive at the 'same' time then ISR is called only once and only one message is read by the routine.  Interrupt is not triggered on the Fill Index > 0 but when the message(s) arrive.   FIFO will not overflow. 

 

_________________________

This is my first adventure into ARM processors.  I've been using dsPICs in the past and until now, when I scratched the surface of ARM world, I didn't even noticed how nice the dsPIC are including CCS IDE for PIC .  I cannot get my head around the ARM compiler syntax at low level libraries.  ASF seems to be a one big bloatware with several nested calls for one simple thing, passing arguments into calls where there is no point of passing anything.  Just slowing down the processor, filling up the stack and using program memory.  The whole HAL idea seems to me like a bit of joke too.  They use different philosophy for HAL for different peripherals.  For example, HAL calls to write to the UART or CDC or CAN or SPI, etc., all seems to have kind of different structure and none of them resemble anything like in sdtio. I looked at the STM32 HAL.  Same thing.  These HAL developers should look at MBED or Arduino etc., to get some ideas how the Hardware Abstraction Layer across different families and platforms might look like.

 

I really like this approach:

https://eewiki.net/display/micro...

This is kind of code that I understand without scratching my head about syntax and it can be happily married with information from datasheet.  Whole CAN library can be written with simple functions and macros such as:

#define kbhit_CAN1() (REG_CAN1_RXF0S&0x0000003fUL)

All I need is just few calls such as getd_CANx(), putd_CANx(), kbhit_CANx(), get_err_CANx(), get_stat_CANx, set_CANx() etc to do all with a CANbus.

___________________________

A view of a hardware engineer keen to write best performing code for hardware and not from a software developer point of view ;o)

In other words, I'm not so thrilled about grammar and punctuation as long as I can say exactly what I want to say even in my own slang :)   

 

 

 

             

Last Edited: Sun. Jul 15, 2018 - 09:17 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Today I rewrote the original: 

int32_t can_async_read(struct can_async_descriptor *const descr, struct can_message *msg);

with a simplified

int32_t getd_CAN1(struct can_message *msg);

here is the code

int32_t getd_CAN1(struct can_message *msg)
{
	struct _can_rx_fifo_entry *f = NULL;

	if (!(REG_CAN1_RXF0S & 0x0000003fUL)) {	
		return ERR_NOT_FOUND;		         // FIFO empty
	}
		
	uint32_t get_index = (REG_CAN1_RXF0S>>8) & (CONF_CAN1_RXF0C_F0S-1);
	f = (struct _can_rx_fifo_entry *)(can1_rx_fifo + (get_index * CONF_CAN1_F0DS));
	REG_CAN1_RXF0A = get_index;

	if (f == NULL) {
		return ERR_NO_RESOURCE;
	}

	if (f->R0.bit.XTD == 1) {
		msg->fmt = CAN_FMT_EXTID;
		msg->id  = f->R0.bit.ID;
		} else {
		msg->fmt = CAN_FMT_STDID;		// A standard identifier is stored into ID[28:18]
		msg->id = f->R0.bit.ID >> 18;
	}

	if (f->R0.bit.RTR == 1) {
		msg->type = CAN_TYPE_REMOTE;
	}

	msg->len =  f->R1.bit.DLC;		// no good for FD

	memcpy(msg->data, f->data, msg->len);
	return ERR_NONE;
}

The execution of original HAL can_async_read() from SAME54 Xplained example took over 18us.  The new getd_CAN1() took less then 10us (no stack used).  Both function use the memcpy() which takes some 5us.  Most likely the function can be shortened by this 5us if the intermediate message f is not used and directly setting the msg instead of the f and then copying f to msg.  I was measuring the timing by toggling LED and looking on CRO.  No debugger. 

 

   

 

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Yes, I am finding the whole Atmel Start a bit inconsistent and bugged. Probably a outsourced project or simply run by junior developers.

 

I am keen not to write everything myself so if there is anything which can help me I will use it, however I need to refer to datasheet to understand their drivers which is really missing the purpose of it.

 

It is nice to know I am not the only one struggling with all this laugh

 

I got the repsone today from Microchip:

We have reported this issue to the concern team. It will be fixed in the future release of Atmel Start.

Hope this helps. Please acknowledge to close this case.

"Future release" not "Next release", so maybe some day... cheeky

 

A view of a hardware engineer keen to write best performing code for hardware and not from a software developer point of view ;o)  

My view is to abstract everything I can, if it is still fast enough:P

 

Good luck and thanks for all the code.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

This is ready to go:)


#include "hpl_can_config.h"
#include "hpl_can_base.h"
extern uint8_t can1_rx_fifo[];

struct can1_rxf0s		// split REG_CAN1_RXF0S into bytes, rather then shifting and masking 32-bit register
{
	uint8_t F0FL;		// FIFO fill index
	uint8_t F0GI;		// FIFO get index
	uint8_t F0PI;		// FIFO put index
	uint8_t ERRORS;		// msg lost and FIFO full flags
};
struct can1_rxf0s *CAN1_RXF0S = (struct can1_rxf0s*)(&REG_CAN1_RXF0S);		// get a reg pointer

#define CAN1_kbhit()		(CAN1_RXF0S->F0FL)
#define CAN1_get_index()	(CAN1_RXF0S->F0GI)
#define CAN1_ack_fifo()		(REG_CAN1_RXF0A=CAN1_RXF0S->F0GI)

int32_t CAN1_get_fifo(struct can_message *msg)
// replaces can_async_read(descr, &msg) from AtmelStart
// return 0 if last FIFO was processed or positive if there still FIFO to be processed or negative on error
{
	if (!CAN1_kbhit())
	{
		return ERR_NOT_FOUND;		// FIFO empty
	}

	struct _can_rx_fifo_entry *fifo_msg = (struct _can_rx_fifo_entry*)(can1_rx_fifo + (CAN1_get_index() * CONF_CAN1_F0DS));	// get a pointer to current msg

	if (fifo_msg->R0.bit.XTD == 1)		// fifo_msg can be processed here rather than copied into return msg
	{
		msg->fmt = CAN_FMT_EXTID;
		msg->id  = fifo_msg->R0.bit.ID;
		} else {
		msg->fmt = CAN_FMT_STDID;	// A standard identifier is stored into ID[28:18]
		msg->id = fifo_msg->R0.bit.ID >> 18;
	}

	if (fifo_msg->R0.bit.RTR == 1)
	{
		msg->type = CAN_TYPE_REMOTE;
	}

	msg->len =  fifo_msg->R1.bit.DLC;	// no good for FD, good for normal CAN

	memcpy(msg->data, fifo_msg->data, msg->len);	//faster would be to directly copy two 32-bit numbers of data instead of calling memcpy() on 8 bytes

	CAN1_ack_fifo();			// release element back to FIFO once processed
				                // ??? maybe few nop() here if the CPU clock is much faster then CAN clock ????
	return CAN1_kbhit();			// returns 0 only if FIFO is empty
}

Getting more and more frustrated with Atmel Start.  Then again, I'm finding things that I didn't see there before.

Last Edited: Sun. Jul 15, 2018 - 09:46 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Unfortunately, our internal team will not give any stipulated time frame for the next release of Atmel Start. It may take one or two quarters form now or even more than that. We are not sure about that. 

Hence, we are not aware of the time frame when the fix will be available for this issue. 

Sorry for the inconvenience caused.

Please get back to us if you have any further clarification.

Looks like Atmel Start is just a hoax. I think they must have prepared something as they are under pressure from other vendors. So they have prepared something which is convoluted and doesn't work very well, however you won't know until you invest some effort into developing using theirs micros...

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I've already pull the plug on Atmel.  Long list.  Atmel might have a problem with actual silicon as well and not only with ASF.  Few weeks of my life that I'll never get back. Only way to properly debug Atmel is with a big baseball bat