Trouble Configuring MSCAN on MCF51JM128

[ChrisS]

Hello everyone,

I'm attempting to set up CAN to pass data between two DEMOJM boards, and suffice to say, it's not working. I have CAN configured in loopback mode, and it appears that my messages are being transmitted, but I never seem to receive them. The receive ISR is never called. Can someone spot what I'm doing wrong?

Thanks in advance!

Chris

/* This function initializes the CAN module. */
void can_init()
{
  // Ask CAN to enter initialization mode.
  CANCTL0_INITRQ = 1;
 
  // Wait until CAN has entered initialization  mode.
  while(CANCTL1_INITAK == 0)
    __RESET_WATCHDOG();

  // Enable MSCAN by asserting CANE.
  CANCTL1_CANE = 1;

  // Use the oscillator clock instead of the bus clock. The bus
  // clock is generated by a PLL, which may create too much
  // jitter to fall within the CAN requirements.
  CANCTL1_CLKsrc=0;
 
  // Set the synchronization jump width to 2 Tq clock cycles.
  CANBTR0_SJW = 0b01;

  // Sample each bit one time.
  CANBTR1_SAMP = 0;
 
  // Set time segment 2 to 2 Tq clock cycles.
  CANBTR1_TSEG_20 = 0b001;
 
  // Set time segment 1 to 4 Tq clock cycles.
  CANBTR1_TSEG_10 = 0b0011;
  
  // Enable loopback self test mode.
  CANCTL1_LOOPB = 1;
 
  // Select eight 8-bit acceptance filters.                 
  CANIDAC_IDAM = 0b10;

  // Accept all messages by programming the mask
  // registers to be all ones.
  CANIDMR0 = 0xff;
  CANIDMR1 = 0xff;
  CANIDMR2 = 0xff;
  CANIDMR3 = 0xff;
  CANIDMR4 = 0xff;
  CANIDMR5 = 0xff;
  CANIDMR6 = 0xff;
  CANIDMR7 = 0xff;
  
  // Clear INITRQ to leave initialization mode and
  // enter normal mode.
  CANCTL0_INITRQ = 0;
 
  // Wait until CAN is in normal mode.
  while(CANCTL1_INITAK == 1)
    __RESET_WATCHDOG();

  // Enable the receiver full/message received interrupt.
  CANRIER_RXFIE = 1;
 
  LED = 0;
}

/* This is the ISR for the CAN receiver full interrupt. */
__interrupt VectorNumber_Vcanrx void canrx_isr()
{
  unsigned int identifier;
  unsigned char data_length; 
  unsigned char data_buffer[8];
 
  LED ^= 1;
 
  // Read the received message from RxFG.
  identifier = (CANRIDR0  3) | (CANRIDR1 & 0xe0);

  data_length = (CANRDLR & 0x0f);

  data_buffer[0] = CANRDSR0;
  data_buffer[1] = CANRDSR1;
  data_buffer[2] = CANRDSR2;
  data_buffer[3] = CANRDSR3;
  data_buffer[4] = CANRDSR4;
  data_buffer[5] = CANRDSR5;
  data_buffer[6] = CANRDSR6;
  data_buffer[7] = CANRDSR7;
 
  // Clear the RXF flag to acknowledge the interrupt and to release
  // the foreground buffer.
  CANRFLG_RXF = 1;
}

/* This function sends a message over CAN. */
int can_send(unsigned int identifier, byte data[8], byte length, byte priority)
{
  // To get the next available transmit buffer, we must
  // read the CANTFLG register and write this value back
  // to the CANTBSEL register.
  CANTBSEL = CANTFLG;
 
  // If there is not an available transmit buffer, it is
  // not possible to proceed.
  if(CANTBSEL == 0x00)
    return -1;

  // Write the identifier into the IDR registers.
  CANTIDR0    = (identifier >> 3) & 0xff;
  CANTIDR1_ID = identifier & 0b111;

  // Write the data into the data segment registers.
  CANTDSR0 = data[0];
  CANTDSR1 = data[1];
  CANTDSR2 = data[2];
  CANTDSR3 = data[3];
  CANTDSR4 = data[4];
  CANTDSR5 = data[5];
  CANTDSR6 = data[6];
  CANTDSR7 = data[7];

  // Write the length into the data length register.
  CANTDLR_DLC = length;

  // Set the priority for this data.
  CANTTBPR = priority;

  // Clear the transmit buffer empty flag to indicate that the
  // message is ready to be sent. (Write of 1 clears the flag.)
  CANTFLG = CANTBSEL;

  return 0;
}


/* Main entry point of the program. */
void main(void) {

  char data[8];
  int i;

  EnableInterrupts;
 
  // MCU/Board Specific configuration
  SOPT1 = 0;              
                               
  //Multi-purpose Clock Generator config register2
  //set HGO (high gain operation),EREFS (oscillator),
  //ERCLKEN (ext clk reference enable)       
  MCGC2 |= 0x26; 
   
  //wait for oscillator to initialize
  while(!MCGSC_OSCINIT);
   
  //Multi-purpose Clock Generator config register1
  MCGC1 = 0x00;
   
  //wait until output of PLL is selected as clock mode
  if(MCGSC & 0x0C)
  {
    for(;;)
      ;
  }
 
  // Make the LED an output, initially off.
  PTEDD_PTEDD2 = 1;
  LED = 1;

  // Make push button 0 an input.
  PTGDD_PTGDD0 = 0;
  PTGPE_PTGPE0 = 1;

  // Initialize the CAN module.
  can_init();  

  // Clear out the data buffer.
  for(i = 0; i  8; i++)
  {
    data[i] = 0x00;
  }

  for(;;) {
 
    // Wait for a button press and debounce.
    while(BUTTON == 1)  
      __RESET_WATCHDOG();

    while(BUTTON == 0)
      __RESET_WATCHDOG();
   
    // Toggle the data.
    data[0] ^= 0x01;
    
    // Send the data.
    can_send(0x12, data, 8, 0);

  }
}

[ChrisS]

I'm still not sure what was wrong with my original code, but I was able to get CAN working by adapting the example given in AN3034. I've attached a zip file with the CodeWarrior project that I made. If you have DEMOJM board, you should be able to load up the code and watch the LEDs blink. Hopefully that will save someone else some time.

Links:

• AN3034 - Using MSCAN on the HCS12 Family
• Software for AN3034

Thanks, 

Chris 

[wrljet]

I was having some trouble moving my CAN code from the 9S08DZ60 to the MCF51JM128 so I've been looking for solutions.

So I happened upon the code you posted 3 years ago, in CANTEST.ZIP.

Acts exactly the same for me on JM128 as my old code.

I get receiver full interrupts and when I pull out the data, once in a while (quite frequently) it contains old/random/incorrect data.  I've tried everything and cannot figure it out.  I suspect there's a bug in the chip.

-Bill

[escu]

Hi Bill,

Did you find a fix ? I also suspect it is a bug in the chip.

I have a similar problem using the chip MCF51AC256,  I have noticed that running at lower speed (250 kbps) the data is more reliable, at higher speed/bus load, random data bytes get overwritten with junk! The most appearing junk value is 127 (0x7F) that randomly overwrites one ore more data bytes in a packet. The last 2 bytes in a 8 bytes data packet have a higher rate of being overwritten.

Did you try to implement id filters ? I am working on this but it looks to me one of the most stupid implementation I have ever seen!

[TomE]

> but it looks to me one of the most stupid implementation I have ever seen!

Try writing code for MCF FlexCAN or for the MCP2515 sometime! At least MSCAN comes with a receive FIFO.

I'll take a guess at your problem as there's an easy mistake to make.

The FIFO isn't like the one on a UART, where there's only one byte per "message" and all you have to do is to read it to clear the way for the next one. There's 13 bytes to read and you have to tell the chip when you've finished. With FlexCAN this signal is when you read the last byte (so you have to read them in the right order). With MSCAN, you wait until CANRFLG[RXF] is set (which will cause an interrupt if enabled) and then you have to read the message from the FIFO BEFORE clearing CANRFLG[RXF]. I assume it will set again if there are other messages in the FIFO.

Note 2 on Table 11-9 says "To ensure data integrity, do not read the receive buffer registers while the RXF flag is cleared.".

Freescale's control registers in the higher-end chips have "write 1 to clear" bits. This allows you to clear individual bits without the risk of a read-modify-write clearing another status bit that turned on during that cycle. This module comes from the HC12 family, and doesn't have that feature, so you have to be very careful with this register. Only write to it ONCE during initialisation and then don't write it again unless you're doing so to clear RXF. Be very careful about handling CANRFLG[OVR]. I can't find anything that says if you have to write this back to zero or if it auto-clears. I would read CANRFLG to find if RXF is set, handle that, and not do anything special about OVR except count it or flag you've missed some data.

Of course you may be getting all this right and it may be some other problem, but I suspect you're reading the FIFO while the chip is loading the next message from the FIFO as you cleared RXF first.

Tom

[escu]

Hi Tom,

Thank you for your reply. I think I am reading all the bytes from the registers and then clear the RXF. Have a look on the code here,maybe it is something I am doing wrong and I can not see it:

byte CAN1_ReadFrame(dword *MessageID,byte *FrameType,byte *FrameFormat,byte *Length,byte *Data)

{

  byte i,j;

  dword tmpId = 0;

  if (!EnUser) {                       /* Is the device disabled by user? */

    return ERR_DISABLED;               /* If yes then error */

  }

  if (CanSerFlag & CANRFLG_OVRIF_MASK) {  /* Is the overrun detected? */

    CanSerFlag &= ~CANRFLG_OVRIF_MASK;    /* Clear the internal overrun flag */

    return ERR_OVERRUN;                /* If yes then error */

  }

  if (CanSerFlag & CANRFLG_CSCIF_MASK) {

      CanSerFlag &= ~CANRFLG_CSCIF_MASK;

  return CANRFLG_CSCIF_MASK;

  }

  if (!(CanSerFlag & FULL_RX_BUF)) {      /* Is the receive buffer empty? */

    return ERR_RXEMPTY;                /* If yes then error */

  }

  if (CanSerFlag != 0x01) {

     i= CanSerFlag;

     CanSerFlag = 0;

     return i;

   }

  ((DwordSwap*)&tmpId)->b.b0 = CANRIDR0;

  ((DwordSwap*)&tmpId)->b.b1 = CANRIDR1;

  ((DwordSwap*)&tmpId)->b.b2 = CANRIDR2;

  ((DwordSwap*)&tmpId)->b.b3 = CANRIDR3;

  if (tmpId & MB_ID_IDE) {

    *MessageID = ((tmpId >> 1) & 0x3FFFFUL) | ((tmpId >> 3) & 0x1FFC0000UL) | CAN_EXTENDED_FRAME_ID; /*

Extended frame */

  }

  else {

    *MessageID = tmpId >> 21;          /* Standard frame */

  }

  if (*MessageID & CAN_EXTENDED_FRAME_ID) {

    *FrameFormat = EXTENDED_FORMAT;

    *FrameType = (byte)((CANRIDR3 & 0x01)? (byte)REMOTE_FRAME : (byte)DATA_FRAME); /* Result the frame

type */

    *MessageID &= ~CAN_EXTENDED_FRAME_ID; /* Remove EXTENDED_FRAME indicator, frame type will be

returned in FrameType parameter */

  }

  else {

    *FrameFormat = STANDARD_FORMAT;

    *FrameType = (byte)((CANRIDR1 & 0x10)? (byte)REMOTE_FRAME : (byte)DATA_FRAME); /* Result the frame

type */

  }

  *Length = (byte)(CANRDLR & 0x0F);    /* Result length of the message */

  if (*FrameType == DATA_FRAME) {      /* Is it "data frame"? */

   for (i=0; i<*Length; i++)

      Data[i] = *((byte *)&CANRDSR0 + i); /* Return received data */

  CanSerFlag &= ~FULL_RX_BUF;             /* Clear flag "full RX buffer" */

  }

  return ERR_OK;                       /* OK */

}

I put a breakpoint once the function returns ERR_OK and inspect the values in the array Data[], and they are sometimes wrong as described.

Any idea ,what else can I try ?

[TomE]

> Any idea ,what else can I try ?

Could you pease try to format your code so it is easier to read in this forum:

https://community.freescale.com/docs/DOC-94181

The only way I've managed to do this consistently is to go into the "Advanced Editor" and then format your code in the "Courier New" font and then "Indent" it as well.

There are way too many problems and outright bugs with that function to try and fix it.

I'll send the buglist as a PRIVATE MESSAGE to you. You'll have to hunt around in your "inbox" to find it.

You're not reading messages from the FIFO when it is FULL or when it isn't a DATA type (an RTR or whatever). You HAVE to read it when there's anything there.

But your main problem, which I missed as well, I found by looking at someone else's code. Why write from scratch when you can find a working example?

Linux/drivers/net/can/mscan/ - Linux Cross Reference - Free Electrons

The above clears the RXF flag by writing ONE to it:

out_8(&regs->canrflg, MSCAN_RXF);

You're trying to do this with this, which can't work:

CanSerFlag &= ~FULL_RX_BUF;

/* Clear flag "full RX buffer" */

In my last post I said:

Freescale's control registers in the higher-end chips have "write 1 to clear" bits.

So does this one, but I missed it as they don't mark this as a "W1C" register bit like they do in other manuals. Instead they say::

15.3.4.1 MSCAN Receiver Flag Register (CANRFLG)

A flag can be cleared only by software (writing a 1 to the corresponding bit position) when the condition

which caused the setting is no longer valid. Every flag has an associated interrupt enable bit in the

CANRIER register.

So that's your main problem. You never cleared that bit except by accident. You should NEVER access that register with the "&=" operator, but always WRITE the bitmap to it that you want to clear.

Tom

[escu]

Hi Tom,

Thank you for your help.

You are right, my problem comes from the following line:

CanSerFlag &= ~FULL_RX_BUF;

I post here the solution and the whole story, maybe it will save some time for a guy like me who spent days to figure out what was going on!

It is not a mistake, as CanSerFlag is not a #define for the register CANRFLG.

This is a flag that is defined outside of the function like this:

static volatile byte CanSerFlag;

In order to receive data from the can-bus, I understand it is mandatory to have the RX Full interrupt enabled, by setting the corresponding flag in CANRIER register:

CANRIER = 0x01;

This is set after the can module has been enabled.

CanSerFlag is used in the ISR code (that runs every time when front RX buffer is full):

ISR(CAN1_InterruptRx)

{

if (CanSerFlag & FULL_RX_BUF) {             /* Is any char already present in the receive buffer? */

CanSerFlag |= CAN_STATUS_OVERRUN_MASK;  /* If yes then set internal flag OVERRUN */

}

CanSerFlag |= FULL_RX_BUF;              /* Set flag "full RX buffer" */

ErrFlag |= (CanSerFlag & 0x83);         /* Add new error flags into the ErrorFlag status variable */

if (CAN1_EnEvent)                       /* Are events enabled? */

    CAN1_OnFullRxBuffer();                  /* If yes then invoke user event */

  //CANRFLG = CANRFLG_RXF_MASK;             /* Reset the reception complete flag and release the RX buffer */

}

In my implementation CAN1_OnFullRxBuffer() was empty, as I do not want to read all the data from the can and not have so many events generated, I only want to have the flag set and at certain point in the main cycle to check for any new can packet (CANRFLG_RXF flag set).

But doing this at the and of the ISR code:

CANRFLG = CANRFLG_RXF_MASK;

was clearing the flag in the register, but not the variable CanSerFlag that I was checking in CAN1_ReadFrame function.

So the solution was to comment the above mentioned line in the ISR and add it in CAN1_ReadFrame just before return.

I have also updated the code in order to handle the situation of receiving a non data frame and also cleaned it up a little:

byte CAN1_ReadFrame(dword *MessageID,byte *FrameType,byte *FrameFormat,byte *Length,byte *Data)

{

  byte i,j;

  dword tmpId = 0;

  if (!EnUser) {                       /* Is the device disabled by user? */

    return ERR_DISABLED;               /* If yes then error */

  }

  if (!(CanSerFlag & FULL_RX_BUF)) {      /* Is the receive buffer empty? */

    return ERR_RXEMPTY;                /* If yes then error */

  }

  ((DwordSwap*)&tmpId)->b.b0 = CANRIDR0;

  ((DwordSwap*)&tmpId)->b.b1 = CANRIDR1;

  ((DwordSwap*)&tmpId)->b.b2 = CANRIDR2;

  ((DwordSwap*)&tmpId)->b.b3 = CANRIDR3;

  if (tmpId & MB_ID_IDE) {

    *MessageID = ((tmpId >> 1) & 0x3FFFFUL) | ((tmpId >> 3) & 0x1FFC0000UL) | CAN_EXTENDED_FRAME_ID; /* Extended frame */

  }

  else {

    *MessageID = tmpId >> 21;          /* Standard frame */

  }

  if (*MessageID & CAN_EXTENDED_FRAME_ID) {

    *FrameFormat = EXTENDED_FORMAT;

    *FrameType = (byte)((CANRIDR3 & 0x01)? (byte)REMOTE_FRAME : (byte)DATA_FRAME); /* Result the frame type */

    *MessageID &= ~CAN_EXTENDED_FRAME_ID; /* Remove EXTENDED_FRAME indicator, frame type will be returned in FrameType parameter */

  }

  else {

    *FrameFormat = STANDARD_FORMAT;

    *FrameType = (byte)((CANRIDR1 & 0x10)? (byte)REMOTE_FRAME : (byte)DATA_FRAME); /* Result the frame type */

  }

  *Length = (byte)(CANRDLR & 0x0F);              /* Result length of the message */

  if (*FrameType == DATA_FRAME) {                /* Is it "data frame"? */

      for (i=0; i<*Length; i++)

          Data[i] = *((byte *)&CANRDSR0 + i);    /* Return received data */

  CanSerFlag &= ~FULL_RX_BUF;                 /* Clear flag "full RX buffer" */

  CANRFLG = CANRFLG_RXF_MASK;

  return ERR_OK;                

    }

  CanSerFlag &= ~FULL_RX_BUF;                     /* Clear flag "full RX buffer" */

  CANRFLG = CANRFLG_RXF_MASK;

  return ERR_VALUE;                               // If we are here we have got no data frame.

}

Now I read correct values, no more junk,  as the full rx flag is cleared in CAN1_ReadFrame only and not any more by the ISR!

Thanks Tom for pinpointing the problem that helped me solving this annoying  issue.

[TomE]

Did you get the private email I sent? You didn't say if you'd managed to read it.

> You are right, my problem comes from the following line:

> CanSerFlag &= ~FULL_RX_BUF;

That's not the problem. Given that is a copy of CANRFLG, that is the correct way clear that bit in that copy.

> This is a flag that is defined outside of the function like this:

> static volatile byte CanSerFlag;

You don't show the code that sets/copies/clears this variable, and you don't show the interrupt service routine so your example code is incomplete and not useful. Don't bother posting it though as I think there are better solutions.

> In order to receive data from the can-bus, I understand it is mandatory to have the

> RX Full interrupt enabled, by setting the corresponding flag in CANRIER register:

You don't need to do that at all. You only need to set the CANRIER bit if your code is written to be interrupted when data arrives. You seem to want to read the messages "when you want to" and not in an interrupt, so to do that all you have to do is poll for the status bits in CANRFLG. If you did that I think it would simplify your code a lot. If your problem is that the CAN interrupts are upsetting some critical timing code elsewhere, then that code should be written to not have critical timing, or it should disable ALL interrupts during the critical period.

If you don't clear the CANRFLG_RXF_MASK bit in CANRFLG during the interrupt service routine, then the interrupt request will never go away and you'll be stuck in the interrupt routine.

Tom

[ChrisS]

Hi kef,

Thanks for your response. I'm actually trying to use loopback mode for the moment, so the first line you mentioned is intentional.

As far as setting the RTR, SRR, and IDE bits, what should the values be?  I tried the following:

  CANTIDR1_IDE = 0;

  CANTIDR3_RTR = 1;

If I understand correctly, that specifies the standard identifier format and indicates that the frame is to be transmitted.  It looks like SRR only applies when you're using extended format.  Still, it doesn't work.  If I'm misunderstanding something, please let me know.

Thanks,

Chris

[kef]

Well, you wrote that you are attempting to pass data between two boards. You can't do that With loopback on.

You can't leave SRR/RTR and IDE bits undefined in Tx buffers. For standard identifiers, IDE should be set to 0. RTR bit should be 0 for normal message, and RTR=1 for remote request.

I'm familiar with MSCAN, but not with JM chip, and can't check what's wrong with your code or maybe JM chip. But the rest of your CAN code looks OK for me (except the habit of using CW bitfields. For example CANRFLG_RXF = 1; clears not only RXF but all CANRFLG flags).

[kef]

You won't be able to receive messages from bus with this

  // Enable loopback self test mode.
  CANCTL1_LOOPB = 1;

Also in your can_send routine

  // Write the identifier into the IDR registers.
  CANTIDR0    = (identifier >> 3) & 0xff;
  CANTIDR1_ID = identifier & 0b111;

That's OK, but you also have to set RTR/SRR and IDE bits.