Skip to content
GitLab
Commit 13a99546 authored by Mahmoud Rushdi's avatar Mahmoud Rushdi Committed by Oliver Horst
Browse files

[fix] Second Peer review findings

parent 05b36608
Hide whitespace changes
Inline Side-by-side
drivers/can.c
View file @ 13a99546
......@@ -44,6 +44,12 @@
#define MAX_NUM_CAN_CONTROLLERS 1U /* Maximum number of supported CAN
Controllers */
/* Mask used to differentiate Standard and Extended CAN frames */
#define CAN_STANDARD_ID_MASK 0xFFFFF800U
/* CAN will be initialized with this baud rate in case of entering Bus off status */
#define CAN_DEFAULT_BAUDRATE 500000U
/******************************************************************************/
/*** Type definitions ***/
/******************************************************************************/
......@@ -115,14 +121,6 @@ static CanInstance_t CanInstance[MAX_NUM_CAN_CONTROLLERS];
/* Instance of the Interrupt Controller driver */
extern XScuGic xInterruptController;
/*
* Shared variables used to test the callbacks.
*/
volatile static int Error; /* Asynchronous error occurred */
volatile static int RecvDone; /* Received a frame */
volatile static int SendDone; /* Frame was sent successfully */
/******************************************************************************/
/*** Prototypes ***/
/******************************************************************************/
......@@ -145,36 +143,44 @@ static void prvSendCanMsg( void* );
* - Correct CAN Bit timing
* And Connect to the Interrupt controller
*/
BaseType_t hal_can_init(hal_io_handle_t DeviceId, uint32_t bitrate)
BaseType_t hal_can_init(hal_io_handle_t deviceId, uint32_t bitrate)
{
XCanPs_Config *CfgPtr;
CanInstance_t *CanInstPtr;
CanInstance_t *canInstPtr;
XCanPs *xilCanPtr;
int Status, i;
XScuGic *IntcInstPtr = & xInterruptController;
XCanPs_Config *cfgPtr;
int status, i;
XScuGic *intcInstPtr = & xInterruptController;
if (DeviceId < MAX_NUM_CAN_CONTROLLERS )
if (deviceId >= MAX_NUM_CAN_CONTROLLERS )
{
CanInstPtr = & CanInstance[DeviceId];
xilCanPtr = &CanInstPtr->canXilinxInst;
// Unsupported deviceId
xil_printf( "CAN: hal_can_init: device id should be less than %d. \n", MAX_NUM_CAN_CONTROLLERS);
return pdFAIL;
}
else
{
canInstPtr = & CanInstance[deviceId];
xilCanPtr = &canInstPtr->canXilinxInst;
CfgPtr = XCanPs_LookupConfig( DeviceId );
if ( !CfgPtr )
cfgPtr = XCanPs_LookupConfig( deviceId );
if ( !cfgPtr )
{
xil_printf( "CAN: hal_can_init: device id can't be resolved. \n");
return pdFAIL;
}
else
{
CanInstPtr->canXilinxInst.CanConfig = *CfgPtr;
canInstPtr->canXilinxInst.CanConfig = *cfgPtr;
}
Status = XCanPs_CfgInitialize(
status = XCanPs_CfgInitialize(
xilCanPtr,
&xilCanPtr->CanConfig,
xilCanPtr->CanConfig.BaseAddr
);
if ( XST_SUCCESS != Status )
if ( XST_SUCCESS != status )
{
xil_printf( "CAN: hal_can_init: Xilinx driver initialization failed. \n");
return pdFAIL;
}
else
......@@ -183,9 +189,10 @@ BaseType_t hal_can_init(hal_io_handle_t DeviceId, uint32_t bitrate)
* Run self-test on the device, which verifies basic sanity of the
* device and the driver.
*/
Status = XCanPs_SelfTest( xilCanPtr );
if ( XST_SUCCESS != Status )
status = XCanPs_SelfTest( xilCanPtr );
if ( XST_SUCCESS != status )
{
xil_printf( "CAN: hal_can_init: Xilinx driver self-test failed. \n");
return pdFAIL;
}
else
......@@ -211,6 +218,8 @@ BaseType_t hal_can_init(hal_io_handle_t DeviceId, uint32_t bitrate)
/* The requested CAN Baud rate is not configured in the lookup table
* Please, add it.
*/
xil_printf( "CAN: hal_can_init: The requested CAN Baud rate is not configured "
"in the lookup table.\n Please, add it. \n");
return pdFAIL;
}
......@@ -230,20 +239,13 @@ BaseType_t hal_can_init(hal_io_handle_t DeviceId, uint32_t bitrate)
* Set interrupt handlers.
*/
XCanPs_SetHandler(xilCanPtr, XCANPS_HANDLER_SEND,
(void *)SendHandler, (void *)CanInstPtr);
(void *)SendHandler, (void *)canInstPtr);
XCanPs_SetHandler(xilCanPtr, XCANPS_HANDLER_RECV,
(void *)RecvHandler, (void *)CanInstPtr);
(void *)RecvHandler, (void *)canInstPtr);
XCanPs_SetHandler(xilCanPtr, XCANPS_HANDLER_ERROR,
(void *)ErrorHandler, (void *)CanInstPtr);
(void *)ErrorHandler, (void *)canInstPtr);
XCanPs_SetHandler(xilCanPtr, XCANPS_HANDLER_EVENT,
(void *)EventHandler, (void *)CanInstPtr);
/*
* Initialize the flags.
*/
SendDone = FALSE;
RecvDone = FALSE;
Error = FALSE;
(void *)EventHandler, (void *)canInstPtr);
/*
* Connect to the interrupt controller.
......@@ -252,17 +254,17 @@ BaseType_t hal_can_init(hal_io_handle_t DeviceId, uint32_t bitrate)
* interrupt for the device occurs, the handler defined above performs
* the specific interrupt processing for the device.
*/
Status = XScuGic_Connect(IntcInstPtr, CanInterruptID[DeviceId],
status = XScuGic_Connect(intcInstPtr, CanInterruptID[deviceId],
(Xil_InterruptHandler)XCanPs_IntrHandler,
(void *)xilCanPtr);
if ( XST_SUCCESS != Status) {
if ( XST_SUCCESS != status) {
return pdFAIL;
}
/*
* Enable the interrupt in the interrupt controller for the CAN device.
*/
XScuGic_Enable(IntcInstPtr, CanInterruptID[DeviceId]);
XScuGic_Enable(intcInstPtr, CanInterruptID[deviceId]);
/*
* Enable all interrupts in CAN device.
......@@ -279,11 +281,6 @@ BaseType_t hal_can_init(hal_io_handle_t DeviceId, uint32_t bitrate)
}
}
}
else
{
// Unsupported DeviceId
return pdFAIL;
}
}
/**
......@@ -292,20 +289,21 @@ BaseType_t hal_can_init(hal_io_handle_t DeviceId, uint32_t bitrate)
* - Resets the CAN controller hardware.
* - Disconnect from the Interrupt controller
*/
BaseType_t hal_can_release(hal_io_handle_t DeviceId)
BaseType_t hal_can_release(hal_io_handle_t deviceId)
{
if (DeviceId < MAX_NUM_CAN_CONTROLLERS )
if (deviceId >= MAX_NUM_CAN_CONTROLLERS )
{
// Disconnect from the interrupt controller.
XScuGic_Disconnect(&xInterruptController, CanInterruptID[DeviceId]);
// Reset the CAN device
XCanPs_Reset( &CanInstance[DeviceId].canXilinxInst );
return pdPASS;
// Unsupported deviceId
xil_printf( "CAN: hal_can_release: device id should be less than %d. \n", MAX_NUM_CAN_CONTROLLERS);
return pdFAIL;
}
else
{
// Unsupported DeviceId
return pdFAIL;
// Disconnect from the interrupt controller.
XScuGic_Disconnect(&xInterruptController, CanInterruptID[deviceId]);
// Reset the CAN device
XCanPs_Reset( &CanInstance[deviceId].canXilinxInst );
return pdPASS;
}
}
......@@ -317,63 +315,53 @@ BaseType_t hal_can_release(hal_io_handle_t DeviceId)
* CAN controller can be accessed by only one freeRTOS task.
* A CAN controller can not be opened twice.
*/
BaseType_t hal_can_open(hal_io_handle_t DeviceId, hal_can_handle_t * canHandle )
BaseType_t hal_can_open(hal_io_handle_t deviceId, hal_can_handle_t * canHandlePtr )
{
CanInstance_t *CanInstPtr;
CanInstance_t *canInstPtr;
BaseType_t xReturned;
if (DeviceId < MAX_NUM_CAN_CONTROLLERS )
if (deviceId >= MAX_NUM_CAN_CONTROLLERS )
{
if (pdFALSE == CanInstance[DeviceId].isCtrlOpen)
// Unsupported deviceId
xil_printf( "CAN: hal_can_open: device id should be less than %d. \n", MAX_NUM_CAN_CONTROLLERS);
return pdFAIL;
}
else
{
if (pdFALSE == CanInstance[deviceId].isCtrlOpen)
{
CanInstPtr = & CanInstance[DeviceId];
*canHandle = CanInstPtr;
canInstPtr = & CanInstance[deviceId];
*canHandlePtr = canInstPtr;
/* Create a receiving queue. */
CanInstPtr->xQueueRx = xQueueCreate( configHAL_CAN_MAX_RX_QUEUE_SIZE , sizeof(hal_can_message_t));
if( NULL == CanInstPtr->xQueueRx )
{
/* Queue was not created and must not be used. */
return pdFAIL;
}
canInstPtr->xQueueRx = xQueueCreate( configHAL_CAN_MAX_RX_QUEUE_SIZE , sizeof(hal_can_message_t));
configASSERT( NULL != canInstPtr->xQueueRx );
/* Create a transmission queue. */
CanInstPtr->xQueueTx = xQueueCreate( configHAL_CAN_MAX_TX_QUEUE_SIZE , sizeof(hal_can_message_t));
if( NULL == CanInstPtr->xQueueTx )
{
/* Queue was not created and must not be used. */
return pdFAIL;
}
canInstPtr->xQueueTx = xQueueCreate( configHAL_CAN_MAX_TX_QUEUE_SIZE , sizeof(hal_can_message_t));
configASSERT( NULL != canInstPtr->xQueueTx );
/* Create a transmission Task */
xReturned = xTaskCreate(
prvSendCanMsg, /* Function that implements the task. */
"Send CAN Frame Task", /* Text name for the task. */
configMINIMAL_STACK_SIZE, /* Stack size in words, not bytes. */
(void *) CanInstPtr, /* Parameter passed into the task. */
(void *) canInstPtr, /* Parameter passed into the task. */
tskIDLE_PRIORITY, /* Priority at which the task is created. */
&CanInstPtr->canTxHandle ); /* Used to pass out the created task's handle. */
if( pdPASS != xReturned )
{
/* The task was not created. */
return pdFAIL;
}
CanInstance[DeviceId].isCtrlOpen = pdTRUE;
&canInstPtr->canTxHandle ); /* Used to pass out the created task's handle. */
configASSERT( pdPASS == xReturned );
CanInstance[deviceId].isCtrlOpen = pdTRUE;
return pdPASS;
}
else
{
/* The controller is already opened. Only one user is supported
* at a time. */
xil_printf( "CAN: hal_can_open: CAN is already opened. Only one user is supported at a time. \n");
return pdFAIL;
}
}
else
{
// Unsupported DeviceId
return pdFAIL;
}
}
/**
......@@ -382,18 +370,18 @@ BaseType_t hal_can_open(hal_io_handle_t DeviceId, hal_can_handle_t * canHandle )
* It destroys the recourses managed by FreeRTOS (queues, tasks)
* and set the CAN port as closed.
*/
BaseType_t hal_can_close( hal_can_handle_t * canHandle )
BaseType_t hal_can_close( hal_can_handle_t * canHandlePtr )
{
CanInstance_t *CanInstPtr = *canHandle;
if (pdTRUE == CanInstPtr->isCtrlOpen)
CanInstance_t *canInstPtr = (CanInstance_t *) *canHandlePtr;
if (pdTRUE == canInstPtr->isCtrlOpen)
{
/* Delete the receiving task */
vTaskDelete( CanInstPtr->canTxHandle );
vTaskDelete( canInstPtr->canTxHandle );
/* Delete Tx and Rx queues */
vQueueDelete( CanInstPtr->xQueueRx );
vQueueDelete( CanInstPtr->xQueueTx );
CanInstPtr->isCtrlOpen = pdFALSE;
*canHandle = NULL;
vQueueDelete( canInstPtr->xQueueRx );
vQueueDelete( canInstPtr->xQueueTx );
canInstPtr->isCtrlOpen = pdFALSE;
*canHandlePtr = NULL;
}
else
{
......@@ -407,11 +395,11 @@ BaseType_t hal_can_close( hal_can_handle_t * canHandle )
*/
BaseType_t hal_can_write(hal_can_handle_t canHandle, hal_can_message_t message)
{
CanInstance_t *CanInstPtr = canHandle;
if (CanInstPtr->isCtrlOpen)
CanInstance_t *canInstPtr = (CanInstance_t *) canHandle;
if (canInstPtr->isCtrlOpen)
{
/* Post the message to the transmitting queue. */
return xQueueSend(CanInstPtr->xQueueTx, &message, 0);
return xQueueSend(canInstPtr->xQueueTx, &message, 0);
}
return pdFAIL;
}
......@@ -421,11 +409,11 @@ BaseType_t hal_can_write(hal_can_handle_t canHandle, hal_can_message_t message)
*/
BaseType_t hal_can_read(hal_can_handle_t canHandle, hal_can_message_t *message)
{
CanInstance_t *CanInstPtr = canHandle;
if (CanInstPtr->isCtrlOpen)
CanInstance_t *canInstPtr = (CanInstance_t *) canHandle;
if (canInstPtr->isCtrlOpen)
{
/* Read a message from the receiving queue. */
return xQueueReceive(CanInstPtr->xQueueRx, message, portMAX_DELAY);
return xQueueReceive(canInstPtr->xQueueRx, message, portMAX_DELAY);
}
return pdFAIL;
}
......@@ -447,15 +435,14 @@ BaseType_t hal_can_read(hal_can_handle_t canHandle, hal_can_message_t *message)
static void SendHandler(void *CallBackRef)
{
/*
* The frame was sent successfully. Notify the task context.
* The frame was sent successfully.
*/
SendDone = TRUE;
CanInstance_t *CanInstPtr = CallBackRef;
CanInstance_t *canInstPtr = (CanInstance_t *) CallBackRef;
/* Notify the transmitting task that a space is free in controller
* Tx queue. This notification might unblock the task, if the
* Tx queue was already full. */
vTaskNotifyGiveFromISR( CanInstPtr->canTxHandle, NULL );
vTaskNotifyGiveFromISR( canInstPtr->canTxHandle, NULL );
}
/*****************************************************************************/
......@@ -475,17 +462,15 @@ static void SendHandler(void *CallBackRef)
******************************************************************************/
static void RecvHandler(void *CallBackRef)
{
CanInstance_t *CanInstPtr = CallBackRef;
XCanPs *xilCanPtr = &CanInstPtr->canXilinxInst;
int Status;
int Index;
CanInstance_t *canInstPtr = (CanInstance_t *) CallBackRef;
XCanPs *xilCanPtr = &canInstPtr->canXilinxInst;
int status;
hal_can_message_t message;
Status = XCanPs_Recv(xilCanPtr, RxFrame);
if ( XST_SUCCESS != Status )
status = XCanPs_Recv(xilCanPtr, RxFrame);
if ( XST_SUCCESS != status )
{
Error = TRUE;
RecvDone = TRUE;
xil_printf( "CAN: RecvHandler: No frame to be received from the controller FIFO. \n");
return;
}
......@@ -495,8 +480,6 @@ static void RecvHandler(void *CallBackRef)
Although it is supported by Xilinx driver. */
xil_printf( "CAN: RecvHandler: Remote request CAN frame is not supported by XME. \n");
Error = TRUE;
RecvDone = TRUE;
}
else
{
......@@ -518,11 +501,8 @@ static void RecvHandler(void *CallBackRef)
*/
memcpy( message.data, &RxFrame[2], sizeof(message.data));
Error = FALSE;
RecvDone = TRUE;
/* Post the message to the receiving queue. */
xQueueSendToBackFromISR( CanInstPtr->xQueueRx, &message, NULL );
xQueueSendToBackFromISR( canInstPtr->xQueueRx, &message, NULL );
}
}
......@@ -546,8 +526,8 @@ static void RecvHandler(void *CallBackRef)
static void ErrorHandler(void *CallBackRef, u32 ErrorMask)
{
CanInstance_t *CanInstPtr = CallBackRef;
XCanPs *xilCanPtr = &CanInstPtr->canXilinxInst;
CanInstance_t *canInstPtr = (CanInstance_t *) CallBackRef;
XCanPs *xilCanPtr = &canInstPtr->canXilinxInst;
if(ErrorMask & XCANPS_ESR_ACKER_MASK) {
/*
......@@ -584,13 +564,6 @@ static void ErrorHandler(void *CallBackRef, u32 ErrorMask)
*/
xil_printf( "CAN: ErrorHandler: CRC Error. \n");
}
/*
* Set the shared variables.
*/
Error = TRUE;
RecvDone = TRUE;
SendDone = TRUE;
}
/*****************************************************************************/
......@@ -624,8 +597,8 @@ static void ErrorHandler(void *CallBackRef, u32 ErrorMask)
******************************************************************************/
static void EventHandler(void *CallBackRef, u32 IntrMask)
{
CanInstance_t *CanInstPtr = CallBackRef;
XCanPs *xilCanPtr = &CanInstPtr->canXilinxInst;
CanInstance_t *canInstPtr = (CanInstance_t *) CallBackRef;
XCanPs *xilCanPtr = &canInstPtr->canXilinxInst;
int i;
if (IntrMask & XCANPS_IXR_BSOFF_MASK) {
......@@ -647,9 +620,10 @@ static void EventHandler(void *CallBackRef, u32 IntrMask)
*/
for (i = 0; i < MAX_NUM_CAN_BAUDRATE; i++)
{
if ( canBitTimingtable[i].can_baudrate == 500000U )
if ( canBitTimingtable[i].can_baudrate == CAN_DEFAULT_BAUDRATE )
break;
}
configASSERT( MAX_NUM_CAN_BAUDRATE > i );
/*
* Setup Baud Rate Prescaler Register (BRPR) and
......@@ -725,18 +699,18 @@ static void EventHandler(void *CallBackRef, u32 IntrMask)
* it to the CAN controller. */
static void prvSendCanMsg( void* arg )
{
CanInstance_t *CanInstPtr = arg;
XCanPs *xilCanPtr = &CanInstPtr->canXilinxInst;
CanInstance_t *canInstPtr = (CanInstance_t *) arg;
XCanPs *xilCanPtr = &canInstPtr->canXilinxInst;
hal_can_message_t message;
while( TRUE )
{
/* Read message from the FreeRTOS CAN Tx queue */
xQueueReceive( CanInstPtr->xQueueTx, &message , portMAX_DELAY );
xQueueReceive( canInstPtr->xQueueTx, &message , portMAX_DELAY );
/* Prepare the CAN frame */
/* Check the message identifier format: Standard, Extended */
if ( 0 == (message.msgID & 0xFFFFF800) )
if ( 0 == (message.msgID & CAN_STANDARD_ID_MASK) )
{
/* Standard CAN frame identifier */
TxFrame[0] = (u32)XCanPs_CreateIdValue(message.msgID, 0, 0, 0, 0);
......@@ -758,7 +732,7 @@ static void prvSendCanMsg( void* arg )
/*
* Wait until the controller TX FIFO has room.
*/
if ( TRUE == XCanPs_IsTxFifoFull(xilCanPtr) )
if ( XCanPs_IsTxFifoFull(xilCanPtr) )
{
/* Block (non-busy waiting) till we have a space in the queue */
/* SendHandler, will inform the task that a message is sent. */
......
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment