Code review! This is ONE function from stm32f1xx_hal_uart.c (v1.0.1, July 2015)
My comments preceded by the triple-semicolons: ";;;"
HAL_StatusTypeDef HAL_UART_Receive(UART_HandleTypeDef *huart, uint8_t *pData, uint16_t Size, uint32_t Timeout)
{
uint16_t* tmp;
uint32_t tmp_state = 0;
tmp_state = huart->State;
if((tmp_state == HAL_UART_STATE_READY) || (tmp_state == HAL_UART_STATE_BUSY_TX))
;;; Why are there states? There is no documentation for what the states
;;; are supposed to accomplish...
;;; I suppose we're using tmp_state because huart isn't "locked" yet?
;;; Does this allow polled, DMA, and ISR based uart IO to occur
;;; "simultaneously" (or "not occur" as the case may be)?
;;; How are those supposed to interact?
{
if((pData == NULL ) || (Size == 0))
{
return HAL_ERROR;
}
/* Process Locked */
__HAL_LOCK(huart);
;;; locked against what?
huart->ErrorCode = HAL_UART_ERROR_NONE;
/* Check if a non-blocking transmit process is ongoing or not */
if(huart->State == HAL_UART_STATE_BUSY_TX)
{
huart->State = HAL_UART_STATE_BUSY_TX_RX;
}
else
{
huart->State = HAL_UART_STATE_BUSY_RX;
}
;;; Now we've changed state. Perhaps this does "real" half-duplex?
huart->RxXferSize = Size;
huart->RxXferCount = Size;
;;; Maybe we copy these so that we can do polled and interrupt driven
;;; IO at the same time? Maybe the states help with that? Oh well,
;;; a few extra copies won't hurt anything, right?
/* Check the remain data to be received */
while(huart->RxXferCount > 0)
{
huart->RxXferCount--;
if(huart->Init.WordLength == UART_WORDLENGTH_9B)
{
if(UART_WaitOnFlagUntilTimeout(huart, UART_FLAG_RXNE, RESET, Timeout) != HAL_OK)
{
return HAL_TIMEOUT;
;;; So where are we now? huart->RxXferSize and huart->RxXferCount
;;; are intermediate values, and we didn't change the state to reflect
;;; that we aborted the input. Doesn't this mean that we'll fail all
;;; subsequent attempts at input?
;;; We also didn't unlock anything!
}
tmp = (uint16_t*) pData ;
;;; for 9bit data, we recast our pointer. Ok, I guess. Since we're
;;; so casual with ASSERT, shouldn't we ASSERT word alignment too?
if(huart->Init.Parity == UART_PARITY_NONE)
{
*tmp = (uint16_t)(huart->Instance->DR & (uint16_t)0x01FF);
pData +=2;
;;; OK, we recieved 9bit data and put it in our 16bit buffer.
;;; (uint16_t)0x01FF seems silly. Doesn't it subsequenlty get promoted
;;; to uint32 to match DR?
}
else
{
*tmp = (uint16_t)(huart->Instance->DR & (uint16_t)0x00FF);
pData +=1;
;;; But WTF did this do? If parity is turned on, we'll just throw
;;; away that 9th bit and store 8bits instead? Doesn't the uart do
;;; 9 bits PLUS parity (10bits total)? If I put it in 9bit mode, don't
;;; I probably want to receive 9bits for each input?
}
;;; Did I mention how great I feel that my program will be cluttered with
;;; (broken?) 9bit UART code even though I'm unlikely to ever use it?
}
else
{
if(UART_WaitOnFlagUntilTimeout(huart, UART_FLAG_RXNE, RESET, Timeout) != HAL_OK)
{
return HAL_TIMEOUT;
;;; more returing without updating State or lock
}
if(huart->Init.Parity == UART_PARITY_NONE)
{
*pData++ = (uint8_t)(huart->Instance->DR & (uint8_t)0x00FF);
;;; OK. masking unnecessary, but probably optimized anyway.
;;; casting "0x00FF" to uint8_t seems particularly silly.
}
else
{
*pData++ = (uint8_t)(huart->Instance->DR & (uint8_t)0x007F);
;;; This is OK for 7 databits + parity, and in fact is necessary since the
;;; UART hardware is documented as leaving the parity bit in DR instead
;;; of stripping it as with many other UARTs.
;;; However, this seems like it's substantially broken for any word size
;;; OTHER than 7 bits. 8 databits+parity will only return 7bits to the user,
;;; databits less than 7 will fail to strip the parity bit.
}
}
}
/* Check if a non-blocking transmit process is ongoing or not */
if(huart->State == HAL_UART_STATE_BUSY_TX_RX)
{
huart->State = HAL_UART_STATE_BUSY_TX;
}
else
{
huart->State = HAL_UART_STATE_READY;
}
/* Process Unlocked */
__HAL_UNLOCK(huart);
;;; more state stuff.
return HAL_OK;
}
else
{
return HAL_BUSY;
}
}
Home
Help
Search
Login
Register
