eldarkg / emdr1986x-std-per-lib

Milandr MCU 1986x Standard Peripherals Library. Mirror:
https://code.launchpad.net/~eldar/emdr1986x-std-per-lib/+git/emdr1986x-std-per-lib
46 stars 29 forks source link

Is function CAN_ITClearRxTxPendingBit correct? #7

Closed in4lio closed 8 years ago

in4lio commented 8 years ago

Hello Eldar,

Won't it be the cause of unwanted retransmission of the same message - call the function CAN_ITClearRxTxPendingBit with flag CAN_STATUS_TX_READYas an argument, like into LoopBack_Polling example?

  ...
  // Send message
  CAN_Transmit(MDR_CAN1, tx_buf, &TxMsg);
        // ==> BUF_CON[tx_buf] = PRIOR_0 | EN | TX_REQ;
  i = 0;
  // Wait for the end of transfer
  while(((CAN_GetStatus(MDR_CAN1) & CAN_STATUS_TX_READY) != RESET) && (i != 0xFFF))
  {
    i++;
  }
  // Send message again??? Next call raises TX_REQ
  CAN_ITClearRxTxPendingBit(MDR_CAN1, tx_buf, CAN_STATUS_TX_READY);
        // ==> BUF_CON[tx_buf] |= TX_REQ;
  ...

Thank you for your time, Vitaly

eldarkg commented 8 years ago

@in4lio you think it is should be?:

void CAN_ITClearRxTxPendingBit(MDR_CAN_TypeDef* CANx, uint32_t BufferNumber, uint32_t Status_Flag)
{
/* ... */
  else if (Status_Flag == CAN_STATUS_TX_READY)
  {
    tmpreg &= ~CAN_STATUS_TX_REQ;
  }
/* ... */
}

You tested this issue?

in4lio commented 8 years ago

I just checked it, the issue proved to be true. As I understand it, we don't need to reset TX interrupt pending bit, it will be cleared up when we start the next transmission.

in4lio commented 8 years ago

We can reduce functionality of this function to avoid potential problems.

/**
  * @brief  Clears the CANx reception buffer interrupt pending bit.
  * @param  CANx: Select the CAN peripheral.
  *         This parameter can be one of the following values:
  *         CAN1, CAN2.
  * @param  BufferNumber: The number of the buffer.
  * @retval None.
  */
void CAN_ITClearRxPendingBit(MDR_CAN_TypeDef* CANx, uint32_t BufferNumber)
{
  uint32_t tmpreg;

  /* Check the parameters */
  assert_param(IS_CAN_ALL_PERIPH(CANx));
  assert_param(IS_CAN_BUFFER(BufferNumber));

  tmpreg = CANx->BUF_CON[BufferNumber];
  tmpreg &= ~CAN_STATUS_RX_FULL;
  CANx->BUF_CON[BufferNumber] = tmpreg;
}

In this case we also need to correct examples.

eldarkg commented 8 years ago

@in4lio I don't want to change API and to break the compatibility with the Milandr library. I think we should to fix issue only. If you tested this issue maybe you will create the pull request with fixing this problem (reset the bit CAN_STATUS_TX_REQ instead of set it)

in4lio commented 8 years ago

Yes, of course, I can create the pull request, but we don't need to reset CAN_STATUS_TX_REQ either - it will be cause of setting TX interrupt pending bit - the opposite effect. We need to remove this operation at all))

in4lio commented 8 years ago

If you approve the following revision I will create a request.

/**
  * @brief  Clears the CANx reception buffer interrupt pending bit,
  *         does nothing if transmission interrupt pending bit is specified.
  * @param  CANx: Select the CAN peripheral.
  *         This parameter can be one of the following values:
  *         CAN1, CAN2.
  * @param  BufferNumber: The number of the buffer
  * @param  Status_Flag: specifies the interrupt pending bit to clear.
  *         This parameter can be of the following values:
            CAN_STATUS_RX_READY:    Flag indicating that there are messages received
            CAN_STATUS_TX_READY:    Flag indicating that there are buffers for transmitting
  * @retval None.
  */
void CAN_ITClearRxTxPendingBit(MDR_CAN_TypeDef* CANx, uint32_t BufferNumber, uint32_t Status_Flag)
{
  uint32_t tmpreg;

  /* Check the parameters */
  assert_param(IS_CAN_ALL_PERIPH(CANx));
  assert_param(IS_CAN_BUFFER(BufferNumber));
  assert_param(IS_CAN_IT_RXTX_FLAG(Status_Flag));

  tmpreg = CANx->BUF_CON[BufferNumber];

  if (Status_Flag == CAN_STATUS_RX_READY)
  {
    tmpreg &= ~CAN_STATUS_RX_FULL;
  }
  else if (Status_Flag == CAN_STATUS_TX_READY)
  {
    /* Setting of TX_REQ bit here, initiates a retransmission of a previous message.
       For this reason, the following line of code has been commented out.
       The transmission interrupt pending bit will be automatically cleared when you
       start the next transmission.
     */
//    tmpreg |= CAN_STATUS_TX_REQ;
  }

  CANx->BUF_CON[BufferNumber] = tmpreg;
}
eldarkg commented 8 years ago

@in4lio You should to mark your comment with the "FIXME" label:

    /* FIXME: Setting of TX_REQ bit here, initiates a retransmission of a previous message.
       For this reason, the following line of code has been commented out.
       The transmission interrupt pending bit will be automatically cleared when you
       start the next transmission.
     */

And then create the PR

eldarkg commented 8 years ago

@in4lio It will be good if you do that like this:

/* FIXME: Setting of TX_REQ bit here, initiates a retransmission of a previous message.
    For this reason, the following line of code has been commented out.
    The transmission interrupt pending bit will be automatically cleared when you
     start the next transmission.
else if (Status_Flag == CAN_STATUS_TX_READY)
{
   tmpreg |= CAN_STATUS_TX_REQ;
}
*/
eldarkg commented 8 years ago

8