Fixes:

CAN RX: ID conversion was incorrect

include/libopencm3/stm32/can.h

@@ -478,7 +478,7 @@ LGPL License Terms @ref lgpl_license
 /* 15:10 Reserved, forced by hardware to 0 */
 
 /* BRP[9:0]: Baud rate prescaler */
-#define CAN_BTR_BRP_MASK		(0x1FF << 0)
+#define CAN_BTR_BRP_MASK		(0x1FFUL << 0)
 
 /* --- CAN_TIxR values ------------------------------------------------------ */
 
@@ -533,11 +533,11 @@ LGPL License Terms @ref lgpl_license
 /* --- CAN_RIxR values ------------------------------------------------------ */
 
 /* STID[10:0]: Standard identifier */
-#define CAN_RIxR_STID_MASK		(0x3FF << 21)
+#define CAN_RIxR_STID_MASK		(0x7FF)
 #define CAN_RIxR_STID_SHIFT		21
 
 /* EXID[15:0]: Extended identifier */
-#define CAN_RIxR_EXID_MASK		(0x1FFFFFF << 3)
+#define CAN_RIxR_EXID_MASK		(0x1FFFFFFF)
 #define CAN_RIxR_EXID_SHIFT		3
 
 /* IDE: Identifier extension */

lib/stm32/can.c

@@ -47,6 +47,18 @@ LGPL License Terms @ref lgpl_license
 #	error "stm32 family not defined."
 #endif
 
+/* Timeout for CAN INIT acknowledge
+ * this value is difficult to define.
+ * INIT is set latest after finishing the current transfer.
+ * Assuming the lowest CAN speed of 100kbps one CAN frame may take about 1.6ms
+ * WAIT loop timeout varies on compiler switches, optimization, CPU architecture
+ * and CPU speed
+ *
+ * The same timeout value is used for leaving INIT where the longest time is
+ * 11 bits(110 us on 100 kbps).
+ */
+#define CAN_MSR_INAK_TIMEOUT 0x0000FFFF
+
 /*-----------------------------------------------------------------------------*/
 /** @brief CAN Reset
 
@@ -88,8 +100,7 @@ int can_init(u32 canport, bool ttcm, bool abom, bool awum, bool nart,
 	     bool rflm, bool txfp, u32 sjw, u32 ts1, u32 ts2, u32 brp,
 	     bool loopback, bool silent)
 {
-	u32 wait_ack = 0x00000000;
+	volatile u32 wait_ack;
-	u32 can_msr_inak_timeout = 0x0000FFFF;
 	int ret = 0;
 
 	/* Exit from sleep mode. */
@@ -99,76 +110,93 @@ int can_init(u32 canport, bool ttcm, bool abom, bool awum, bool nart,
 	CAN_MCR(canport) |= CAN_MCR_INRQ;
 
 	/* Wait for acknowledge. */
-	while ((wait_ack != can_msr_inak_timeout) &&
+	wait_ack = CAN_MSR_INAK_TIMEOUT;
+	while ((--wait_ack) &&
 	       ((CAN_MSR(canport) & CAN_MSR_INAK) != CAN_MSR_INAK)) {
-		wait_ack++;
 	}
 
 	/* Check the acknowledge. */
-	if ((CAN_MSR(canport) & CAN_MSR_INAK) != CAN_MSR_INAK)
+	if ((CAN_MSR(canport) & CAN_MSR_INAK) != CAN_MSR_INAK){
 		return 1;
+	}
 
 	/* clear can timing bits */
 	CAN_BTR(canport) = 0;
 
 	/* Set the automatic bus-off management. */
-	if (ttcm)
+	if (ttcm) {
 		CAN_MCR(canport) |= CAN_MCR_TTCM;
-	else
+	}
+	else {
 		CAN_MCR(canport) &= ~CAN_MCR_TTCM;
+	}
 
-	if (abom)
+	if (abom) {
 		CAN_MCR(canport) |= CAN_MCR_ABOM;
-	else
+	}
+	else {
 		CAN_MCR(canport) &= ~CAN_MCR_ABOM;
+	}
 
-	if (awum)
+	if (awum) {
 		CAN_MCR(canport) |= CAN_MCR_AWUM;
-	else
+	}
+	else {
 		CAN_MCR(canport) &= ~CAN_MCR_AWUM;
+	}
 
-	if (nart)
+	if (nart) {
 		CAN_MCR(canport) |= CAN_MCR_NART;
-	else
+	}
+	else{
 		CAN_MCR(canport) &= ~CAN_MCR_NART;
+	}
 
-	if (rflm)
+	if (rflm) {
 		CAN_MCR(canport) |= CAN_MCR_RFLM;
-	else
+	}
+	else {
 		CAN_MCR(canport) &= ~CAN_MCR_RFLM;
+	}
 
-	if (txfp)
+	if (txfp) {
 		CAN_MCR(canport) |= CAN_MCR_TXFP;
-	else
+	}
+	else {
 		CAN_MCR(canport) &= ~CAN_MCR_TXFP;
+	}
 
-	if (silent)
+	if (silent) {
 		CAN_BTR(canport) |= CAN_BTR_SILM;
-	else
+	}
+	else {
 		CAN_BTR(canport) &= ~CAN_BTR_SILM;
+	}
 
-	if (loopback)
+	if (loopback) {
 		CAN_BTR(canport) |= CAN_BTR_LBKM;
-	else
+	}
+	else {
 		CAN_BTR(canport) &= ~CAN_BTR_LBKM;
+	}
 
 
 	/* Set bit timings. */
 	CAN_BTR(canport) |= sjw | ts2 | ts1 |
-		           (u32)(CAN_BTR_BRP_MASK & (brp - 1));
+		           ((brp - 1ul) & CAN_BTR_BRP_MASK);
 
 	/* Request initialization "leave". */
 	CAN_MCR(canport) &= ~CAN_MCR_INRQ;
 
 	/* Wait for acknowledge. */
-	wait_ack = 0x00000000;
+	wait_ack = CAN_MSR_INAK_TIMEOUT;
-	while ((wait_ack != can_msr_inak_timeout) &&
+	while ((--wait_ack) &&
 	       ((CAN_MSR(canport) & CAN_MSR_INAK) == CAN_MSR_INAK)) {
-		wait_ack++;
 	}
 
-	if ((CAN_MSR(canport) & CAN_MSR_INAK) == CAN_MSR_INAK)
+	if ((CAN_MSR(canport) & CAN_MSR_INAK) == CAN_MSR_INAK) {
 		ret = 1;
+	}
 
 	return ret;
 }
@@ -221,13 +249,15 @@ void can_filter_init(u32 canport, u32 nr, bool scale_32bit, bool id_list_mode,
 	CAN_FiR2(canport, nr) = fr2;
 
 	/* Select FIFO0 or FIFO1 as filter assignement. */
-	if (fifo)
+	if (fifo) {
 		CAN_FFA1R(canport) |= filter_select_bit;  /* FIFO1 */
-	else
+	}
+	else {
 		CAN_FFA1R(canport) &= ~filter_select_bit; /* FIFO0 */
-
+	}
-	if (enable)
+	if (enable) {
 		CAN_FA1R(canport) |= filter_select_bit; /* Activate filter. */
+	}
 
 	/* Request initialization "leave". */
 	CAN_FMR(canport) &= ~CAN_FMR_FINIT;
@@ -431,10 +461,12 @@ int can_transmit(u32 canport, u32 id, bool ext, bool rtr, u8 length, u8 *data)
  */
 void can_fifo_release(u32 canport, u8 fifo)
 {
-	if (fifo == 0)
+	if (fifo == 0) {
 		CAN_RF0R(canport) |= CAN_RF1R_RFOM1;
-	else
+	}
+	else {
 		CAN_RF1R(canport) |= CAN_RF1R_RFOM1;
+	}
 }
 
 /*-----------------------------------------------------------------------------*/
@@ -466,22 +498,18 @@ void can_receive(u32 canport, u8 fifo, bool release, u32 *id, bool *ext,
 	if (CAN_RIxR(canport, fifo_id) & CAN_RIxR_IDE) {
 		*ext = true;
 		/* Get extended CAN ID. */
-		*id = ((CAN_RIxR(canport, fifo_id) & CAN_RIxR_EXID_MASK) >>
+		*id = (CAN_RIxR(canport, fifo_id) >> CAN_RIxR_EXID_SHIFT) & CAN_RIxR_EXID_MASK;
-			CAN_RIxR_EXID_SHIFT);
 	} else {
 		*ext = false;
 		/* Get standard CAN ID. */
-		*id = ((CAN_RIxR(canport, fifo_id) & CAN_RIxR_STID_MASK) >>
+		*id = (CAN_RIxR(canport, fifo_id) >> CAN_RIxR_STID_SHIFT) & CAN_RIxR_STID_MASK;
-			CAN_RIxR_STID_SHIFT);
 	}
 
 	/* Get remote transmit flag. */
-	if (CAN_RIxR(canport, fifo_id) & CAN_RIxR_RTR)
+	if (CAN_RIxR(canport, fifo_id) & CAN_RIxR_RTR)	{
-	{
 		*rtr = true;
 	}
-	else
+	else {
-	{
 		*rtr = false;
 	}
 
@@ -500,11 +528,11 @@ void can_receive(u32 canport, u8 fifo, bool release, u32 *id, bool *ext,
 	/* Get data.
 	 * Byte wise copy is needed because we do not know the alignment
 	 * of the input buffer.
-	 * Here copying 8 bytes each is faster than using loop
+	 * Here copying 8 bytes unconditionally is faster than using loop
 	 *
 	 * It is OK to copy all 8 bytes because the upper layer must be
-	 * prepared that the data length of the CAN frame is bigger
+	 * prepared for data length bigger expected.
-	 * than expected. In contrary the driver has no clue what is expected.
+	 * In contrary the driver has no information about the intended size.
 	 * This could be different if the max length would be handed over
 	 * to the function, but it is not the case
 	 */