Volatile variable incorrect after interrupt callback

[JBrow]

I'm currently attempting to get I2C and UART communication up between a chain of processors like so NXP chip <- UART -> STM32L063 <- (slave) I2C (master) -> STM32F030. I am using interrupts to handle UART and I2C Rx and Tx callbacks, you can see my code below. UART is working no problem, the issue I am having is with I2C. My I2C protocol has master writing to slave and then reading from slave. This repeats every 10ms. It will work for a while and then stop after the master sends the read request with the slave's address over the I2C bus. Using a logic analyzer and oscilloscope, it's clear that the slave is clock stretching the bus as it waits to respond but never does. Resetting the slave generates a nack on the line which further shows the slave is holding up the bus.

I've narrowed down the issue to being that the app_flags |= APP_FLAG_PROCESS_I2C is not persisting outside of the HAL_I2C_SlaveRxCpltCallback callback when I2C fails. I verify the line gets called by seeing that both GPIO toggles surrounding it occur on the logic analyzer. If I set a breakpoint where this flag is checked after I2C fails I can see that the flag is not set. Jumping into the block with GDB gets I2C communication to continue as normal so it is clear that this flag getting set is not persisting and is causing the I2C bus to halt since the HAL_I2C_Slave_Transmit_IT function in the block is not getting called. There are also GPIO toggles in the block that is not being entered that are not triggered when I2C stops which also shows this block is not being entered.

Other info:

All interrupts share the same priority.

I am using STM32Cube

My question is this, why isn't this flag persisting? app_flags is an uint16_t which should perform reads/writes atomically on a 32bit system. Its also volatile so it should be getting read directly from memory every time it is used.

You can also see the app_flags_i2c variable which directly mirrors app_flags with the only difference is that it is not checked against in the main while(1) loop. Oddly, this variable does have the flag set correctly when i2c fails and app_flags doesn't have the flag set. Changing to check against app_flags_i2c instead of app_flags causes their behavior to switch (i.e. app_flags set correctly, app_flags_i2c not).

Code in comments

[JBrow]

/* USER CODE BEGIN Header */
/**
 ******************************************************************************
 * @file           : main.c
 * @brief          : Main program body
 ******************************************************************************
 * @attention
 *
 * <h2><center>&copy; Copyright (c) 2019 STMicroelectronics.
 * All rights reserved.</center></h2>
 *
 * This software component is licensed by ST under BSD 3-Clause license,
 * the "License"; You may not use this file except in compliance with the
 * License. You may obtain a copy of the License at:
 *                        opensource.org/licenses/BSD-3-Clause
 *
 ******************************************************************************
 */
/* USER CODE END Header */
 
/* Includes ------------------------------------------------------------------*/
#include "main.h"
 
/* Private includes ----------------------------------------------------------*/
/* USER CODE BEGIN Includes */
#include <comm_psc.h>
#include <lib_briskets.h>
#include <lib_briskets_utils.h>
#include <stm32_usart_dma_queue.h>
#include <string.h>
/* USER CODE END Includes */
 
/* Private typedef -----------------------------------------------------------*/
/* USER CODE BEGIN PTD */
 
/* USER CODE END PTD */
 
/* Private define ------------------------------------------------------------*/
/* USER CODE BEGIN PD */
 
#define BRISKETS_PACKET_SIZE sizeof(BRISKETS_PACKET)
 
/* USER CODE END PD */
 
/* Private macro -------------------------------------------------------------*/
/* USER CODE BEGIN PM */
 
/* USER CODE END PM */
 
/* Private variables ---------------------------------------------------------*/
CRC_HandleTypeDef hcrc;
 
I2C_HandleTypeDef hi2c1;
 
TIM_HandleTypeDef htim6;
 
UART_HandleTypeDef huart2;
DMA_HandleTypeDef hdma_usart2_tx;
 
/* USER CODE BEGIN PV */
CRC_HandleTypeDef*        st_crc         = &hcrc;
UART_HandleTypeDef*       st_uart_psc    = &huart2;
static I2C_HandleTypeDef* st_i2c_ncc_cp  = &hi2c1;
static TIM_HandleTypeDef* st_tim_heartbt = &htim6;
 
volatile uint8_t ncc_state = 0x00;
 
volatile uint8_t ncc_cp_state = 0x00;
 
static uint8_t i2c_ncc_cp_rx[BRISKETS_PACKET_SIZE];
 
static BRISKETS_PACKET packet;
 
static BRISKETS_PACKET cpacket;
 
/* Internal state machine variables*/
static volatile uint16_t app_flags_i2c = 0x0000;
static volatile uint16_t app_flags = 0x0000;
  #define APP_FLAG_DRIVE_COMM_PSC    0x0001
  #define APP_FLAG_ENABLE_COMM_PSC   0x0002
  #define APP_FLAG_DRIVE_HEARTBEAT   0x0004
  #define APP_FLAG_PROCESS_BRISKETS  0x0008
 
/* 64bit SysTick used for LIB_CHEERIOS */
static uint64_t systick = 0;
/* USER CODE END PV */
 
/* Private function prototypes -----------------------------------------------*/
void SystemClock_Config(void);
static void MX_GPIO_Init(void);
static void MX_DMA_Init(void);
static void MX_USART2_UART_Init(void);
static void MX_CRC_Init(void);
static void MX_I2C1_Init(void);
static void MX_TIM6_Init(void);
/* USER CODE BEGIN PFP */
 
/* USER CODE END PFP */
 
/* Private user code ---------------------------------------------------------*/
/* USER CODE BEGIN 0 */
void SystickCallback()
{
  ++systick;
 
  if ( (app_flags & APP_FLAG_ENABLE_COMM_PSC) != 0 )
  {
    COMM_PSC_UpdateTimeTick( systick );
  }
}
 
void HAL_UART_RxCpltCallback( UART_HandleTypeDef* huart )
{
  if ( huart == st_uart_psc )
  {
    /* Retrieve the data from the PSC UART Rx line and */
    /* trigger the main loop to drive comm_psc         */
    COMM_PSC_UART_RxCpltCallback();
    app_flags |= APP_FLAG_DRIVE_COMM_PSC;
    app_flags_i2c |= APP_FLAG_DRIVE_COMM_PSC;
  }
}
 
void HAL_UART_TxCpltCallback( UART_HandleTypeDef* huart )
{
  STM32DMAQ_TxCpltCallback( huart );
}
 
void HAL_UART_ErrorCallback( UART_HandleTypeDef* huart )
{
  if ( huart == st_uart_psc )
  {
    COMM_PSC_UART_ErrorCallback();
  }
}
 
void HAL_I2C_SlaveRxCpltCallback( I2C_HandleTypeDef *hi2c )
{
  if ( hi2c == st_i2c_ncc_cp )
  {
    memcpy( &cpacket, i2c_ncc_cp_rx, sizeof(cpacket) );
 
    HAL_GPIO_TogglePin(GPIOB, GPIO_PIN_0);
 
    app_flags |= APP_FLAG_PROCESS_BRISKETS;
    app_flags_i2c |= APP_FLAG_PROCESS_BRISKETS;
 
    HAL_GPIO_TogglePin(GPIOB, GPIO_PIN_10);
  }
}
 
void HAL_I2C_SlaveTxCpltCallback( I2C_HandleTypeDef *hi2c )
{
  memset( i2c_ncc_cp_rx, 0, sizeof(i2c_ncc_cp_rx) );
  HAL_I2C_Slave_Receive_IT( st_i2c_ncc_cp, i2c_ncc_cp_rx, sizeof(i2c_ncc_cp_rx) );
}
 
void HAL_TIM_PeriodElapsedCallback( TIM_HandleTypeDef *htim )
{
  if ( htim->Instance == st_tim_heartbt->Instance )
  {
    app_flags |= APP_FLAG_DRIVE_HEARTBEAT;
    app_flags_i2c |= APP_FLAG_DRIVE_HEARTBEAT;
  }
}
 
void HandleNccCpStateChange()
{
  // TODO
}
 
/* USER CODE END 0 */
 
/**
  * @brief  The application entry point.
  * @retval int
  */
int main(void)
{
  /* USER CODE BEGIN 1 */
 
  /* USER CODE END 1 */
  
 
  /* MCU Configuration--------------------------------------------------------*/
 
  /* Reset of all peripherals, Initializes the Flash interface and the Systick. */
  HAL_Init();
 
  /* USER CODE BEGIN Init */
 
  /* USER CODE END Init */
 
  /* Configure the system clock */
  SystemClock_Config();
 
  /* USER CODE BEGIN SysInit */
 
  /* USER CODE END SysInit */
 
  /* Initialize all configured peripherals */
  MX_GPIO_Init();
  MX_DMA_Init();
  MX_USART2_UART_Init();
  MX_CRC_Init();
  MX_I2C1_Init();
  MX_TIM6_Init();
  /* USER CODE BEGIN 2 */
 
  /* Initialize LIB_CHEERIOS for the PSC UART */
  if ( COMM_PSC_Init() != 0 )
  {
    Error_Handler();
  }
 
  /* Enable LIB_CHEERIOS for the PSC UART */
  app_flags |= APP_FLAG_ENABLE_COMM_PSC;
 
  memset( i2c_ncc_cp_rx, 0, sizeof(i2c_ncc_cp_rx) );
  memset( &cpacket, 0, sizeof(cpacket) );
 
  HAL_I2C_Slave_Receive_IT( st_i2c_ncc_cp, i2c_ncc_cp_rx, sizeof(i2c_ncc_cp_rx) );
 
  /* Reset the SR (update event) register to prevent the timer from */
  /* firing immediately.  After the HAL is configured for the timer */
  /* instance, the SR register's default value is 1 which will make */
  /* the timer fire immediately upon start.                         */
  st_tim_heartbt->Instance->SR = 0;
 
  /* Start the heartbeat timer */
  HAL_TIM_Base_Start_IT( st_tim_heartbt );
 
  /* USER CODE END 2 */
 
  /* Infinite loop */
  /* USER CODE BEGIN WHILE */
  while ( 1 )
  {
    if ( (app_flags & APP_FLAG_PROCESS_BRISKETS) != 0 )
    {
      HAL_GPIO_TogglePin(GPIOC, GPIO_PIN_13);
 
      app_flags &= ~APP_FLAG_PROCESS_BRISKETS;
      app_flags_i2c &= ~APP_FLAG_PROCESS_BRISKETS;
 
	  // Grab the cp's state and send our response back to the cp
	  ncc_cp_state = cpacket.payload.state.ncc_cp_state;
 
	  memset( &packet, 0, sizeof(packet) );
 
	  packet.version                    = PKT_VERSION_ONE;
	  packet.opcode                     = OPCODE_STATE;
	  packet.payload.state.ncc_state    = ncc_state;
	  packet.payload.state.ncc_cp_state = ncc_cp_state;
	  packet.crc                        = HAL_CRC_Calculate( st_crc, (uint32_t*)&packet, sizeof(packet) - sizeof(uint32_t) );
 
	  HAL_GPIO_TogglePin(GPIOC, GPIO_PIN_13);
 
	  HAL_I2C_Slave_Transmit_IT( st_i2c_ncc_cp, (uint8_t*)&packet, sizeof(packet) );
 
	  HAL_GPIO_TogglePin(GPIOC, GPIO_PIN_13);
    }
 
    if ( (app_flags & APP_FLAG_DRIVE_HEARTBEAT) != 0 )
    {
      app_flags &= ~APP_FLAG_DRIVE_HEARTBEAT;
      app_flags_i2c &= ~APP_FLAG_DRIVE_HEARTBEAT;
 
      /* Drive the heartbeat to the PSC as long as comms are enabled */
      if ( (app_flags & APP_FLAG_ENABLE_COMM_PSC) != 0 )
        COMM_PSC_Heartbeat();
    }
 
    if ( (app_flags & APP_FLAG_DRIVE_COMM_PSC) != 0 )
    {
	  app_flags &= ~APP_FLAG_DRIVE_COMM_PSC;
	  app_flags_i2c &= ~APP_FLAG_DRIVE_COMM_PSC;
      COMM_PSC_Drive();
    }
    /* USER CODE END WHILE */
 
    /* USER CODE BEGIN 3 */
  }
  /* USER CODE END 3 */
}

[JBrow]

 
/**
  * @brief System Clock Configuration
  * @retval None
  */
void SystemClock_Config(void)
{
  RCC_OscInitTypeDef RCC_OscInitStruct = {0};
  RCC_ClkInitTypeDef RCC_ClkInitStruct = {0};
  RCC_PeriphCLKInitTypeDef PeriphClkInit = {0};
 
  /** Configure the main internal regulator output voltage 
  */
  __HAL_PWR_VOLTAGESCALING_CONFIG(PWR_REGULATOR_VOLTAGE_SCALE1);
  /** Initializes the CPU, AHB and APB busses clocks 
  */
  RCC_OscInitStruct.OscillatorType = RCC_OSCILLATORTYPE_HSI;
  RCC_OscInitStruct.HSIState = RCC_HSI_ON;
  RCC_OscInitStruct.HSICalibrationValue = RCC_HSICALIBRATION_DEFAULT;
  RCC_OscInitStruct.PLL.PLLState = RCC_PLL_ON;
  RCC_OscInitStruct.PLL.PLLSource = RCC_PLLSOURCE_HSI;
  RCC_OscInitStruct.PLL.PLLMUL = RCC_PLLMUL_4;
  RCC_OscInitStruct.PLL.PLLDIV = RCC_PLLDIV_2;
  if (HAL_RCC_OscConfig(&RCC_OscInitStruct) != HAL_OK)
  {
    Error_Handler();
  }
  /** Initializes the CPU, AHB and APB busses clocks 
  */
  RCC_ClkInitStruct.ClockType = RCC_CLOCKTYPE_HCLK|RCC_CLOCKTYPE_SYSCLK
                              |RCC_CLOCKTYPE_PCLK1|RCC_CLOCKTYPE_PCLK2;
  RCC_ClkInitStruct.SYSCLKSource = RCC_SYSCLKSOURCE_PLLCLK;
  RCC_ClkInitStruct.AHBCLKDivider = RCC_SYSCLK_DIV1;
  RCC_ClkInitStruct.APB1CLKDivider = RCC_HCLK_DIV1;
  RCC_ClkInitStruct.APB2CLKDivider = RCC_HCLK_DIV1;
 
  if (HAL_RCC_ClockConfig(&RCC_ClkInitStruct, FLASH_LATENCY_1) != HAL_OK)
  {
    Error_Handler();
  }
  PeriphClkInit.PeriphClockSelection = RCC_PERIPHCLK_USART2|RCC_PERIPHCLK_I2C1;
  PeriphClkInit.Usart2ClockSelection = RCC_USART2CLKSOURCE_PCLK1;
  PeriphClkInit.I2c1ClockSelection = RCC_I2C1CLKSOURCE_PCLK1;
  if (HAL_RCCEx_PeriphCLKConfig(&PeriphClkInit) != HAL_OK)
  {
    Error_Handler();
  }
}
 
/**
  * @brief CRC Initialization Function
  * @param None
  * @retval None
  */
static void MX_CRC_Init(void)
{
 
  /* USER CODE BEGIN CRC_Init 0 */
 
  /* USER CODE END CRC_Init 0 */
 
  /* USER CODE BEGIN CRC_Init 1 */
 
  /* USER CODE END CRC_Init 1 */
  hcrc.Instance = CRC;
  hcrc.Init.DefaultPolynomialUse = DEFAULT_POLYNOMIAL_ENABLE;
  hcrc.Init.DefaultInitValueUse = DEFAULT_INIT_VALUE_DISABLE;
  hcrc.Init.InitValue = 0;
  hcrc.Init.InputDataInversionMode = CRC_INPUTDATA_INVERSION_BYTE;
  hcrc.Init.OutputDataInversionMode = CRC_OUTPUTDATA_INVERSION_ENABLE;
  hcrc.InputDataFormat = CRC_INPUTDATA_FORMAT_BYTES;
  if (HAL_CRC_Init(&hcrc) != HAL_OK)
  {
    Error_Handler();
  }
  /* USER CODE BEGIN CRC_Init 2 */
 
  /* USER CODE END CRC_Init 2 */
 
}
 
/**
  * @brief I2C1 Initialization Function
  * @param None
  * @retval None
  */
static void MX_I2C1_Init(void)
{
 
  /* USER CODE BEGIN I2C1_Init 0 */
 
  /* USER CODE END I2C1_Init 0 */
 
  /* USER CODE BEGIN I2C1_Init 1 */
 
  /* USER CODE END I2C1_Init 1 */
  hi2c1.Instance = I2C1;
  hi2c1.Init.Timing = 0x00707CBB;
  hi2c1.Init.OwnAddress1 = 42;
  hi2c1.Init.AddressingMode = I2C_ADDRESSINGMODE_7BIT;
  hi2c1.Init.DualAddressMode = I2C_DUALADDRESS_DISABLE;
  hi2c1.Init.OwnAddress2 = 0;
  hi2c1.Init.OwnAddress2Masks = I2C_OA2_NOMASK;
  hi2c1.Init.GeneralCallMode = I2C_GENERALCALL_DISABLE;
  hi2c1.Init.NoStretchMode = I2C_NOSTRETCH_DISABLE;
  if (HAL_I2C_Init(&hi2c1) != HAL_OK)
  {
    Error_Handler();
  }
  /** Configure Analogue filter 
  */
  if (HAL_I2CEx_ConfigAnalogFilter(&hi2c1, I2C_ANALOGFILTER_ENABLE) != HAL_OK)
  {
    Error_Handler();
  }
  /** Configure Digital filter 
  */
  if (HAL_I2CEx_ConfigDigitalFilter(&hi2c1, 0) != HAL_OK)
  {
    Error_Handler();
  }
  /* USER CODE BEGIN I2C1_Init 2 */
 
  /* USER CODE END I2C1_Init 2 */
 
}
 
/**
  * @brief TIM6 Initialization Function
  * @param None
  * @retval None
  */
static void MX_TIM6_Init(void)
{
 
  /* USER CODE BEGIN TIM6_Init 0 */
 
  /* USER CODE END TIM6_Init 0 */
 
  TIM_MasterConfigTypeDef sMasterConfig = {0};
 
  /* USER CODE BEGIN TIM6_Init 1 */
 
  /* USER CODE END TIM6_Init 1 */
  htim6.Instance = TIM6;
  htim6.Init.Prescaler = 32000;
  htim6.Init.CounterMode = TIM_COUNTERMODE_UP;
  htim6.Init.Period = 1000;
  htim6.Init.AutoReloadPreload = TIM_AUTORELOAD_PRELOAD_ENABLE;
  if (HAL_TIM_Base_Init(&htim6) != HAL_OK)
  {
    Error_Handler();
  }
  sMasterConfig.MasterOutputTrigger = TIM_TRGO_RESET;
  sMasterConfig.MasterSlaveMode = TIM_MASTERSLAVEMODE_DISABLE;
  if (HAL_TIMEx_MasterConfigSynchronization(&htim6, &sMasterConfig) != HAL_OK)
  {
    Error_Handler();
  }
  /* USER CODE BEGIN TIM6_Init 2 */
 
  /* USER CODE END TIM6_Init 2 */
 
}
 
/**
  * @brief USART2 Initialization Function
  * @param None
  * @retval None
  */
static void MX_USART2_UART_Init(void)
{
 
  /* USER CODE BEGIN USART2_Init 0 */
 
  /* USER CODE END USART2_Init 0 */
 
  /* USER CODE BEGIN USART2_Init 1 */
 
  /* USER CODE END USART2_Init 1 */
  huart2.Instance = USART2;
  huart2.Init.BaudRate = 9600;
  huart2.Init.WordLength = UART_WORDLENGTH_8B;
  huart2.Init.StopBits = UART_STOPBITS_1;
  huart2.Init.Parity = UART_PARITY_NONE;
  huart2.Init.Mode = UART_MODE_TX_RX;
  huart2.Init.HwFlowCtl = UART_HWCONTROL_NONE;
  huart2.Init.OverSampling = UART_OVERSAMPLING_16;
  huart2.Init.OneBitSampling = UART_ONE_BIT_SAMPLE_DISABLE;
  huart2.AdvancedInit.AdvFeatureInit = UART_ADVFEATURE_RXOVERRUNDISABLE_INIT;
  huart2.AdvancedInit.OverrunDisable = UART_ADVFEATURE_OVERRUN_DISABLE;
  if (HAL_UART_Init(&huart2) != HAL_OK)
  {
    Error_Handler();
  }
  /* USER CODE BEGIN USART2_Init 2 */
 
  /* USER CODE END USART2_Init 2 */
 
}
 
/** 
  * Enable DMA controller clock
  */
static void MX_DMA_Init(void) 
{
  /* DMA controller clock enable */
  __HAL_RCC_DMA1_CLK_ENABLE();
 
  /* DMA interrupt init */
  /* DMA1_Channel4_5_6_7_IRQn interrupt configuration */
  HAL_NVIC_SetPriority(DMA1_Channel4_5_6_7_IRQn, 0, 0);
  HAL_NVIC_EnableIRQ(DMA1_Channel4_5_6_7_IRQn);
 
}
 
/**
  * @brief GPIO Initialization Function
  * @param None
  * @retval None
  */
static void MX_GPIO_Init(void)
{
  GPIO_InitTypeDef GPIO_InitStruct = {0};
 
  /* GPIO Ports Clock Enable */
  __HAL_RCC_GPIOC_CLK_ENABLE();
  __HAL_RCC_GPIOA_CLK_ENABLE();
  __HAL_RCC_GPIOB_CLK_ENABLE();
 
  /*Configure GPIO pin Output Level */
  HAL_GPIO_WritePin(GPIOC, GPIO_PIN_13, GPIO_PIN_RESET);
 
  /*Configure GPIO pin Output Level */
  HAL_GPIO_WritePin(GPIOB, GPIO_PIN_0|GPIO_PIN_10, GPIO_PIN_RESET);
 
  /*Configure GPIO pin : PC13 */
  GPIO_InitStruct.Pin = GPIO_PIN_13;
  GPIO_InitStruct.Mode = GPIO_MODE_OUTPUT_PP;
  GPIO_InitStruct.Pull = GPIO_NOPULL;
  GPIO_InitStruct.Speed = GPIO_SPEED_FREQ_LOW;
  HAL_GPIO_Init(GPIOC, &GPIO_InitStruct);
 
  /*Configure GPIO pins : PB0 PB10 */
  GPIO_InitStruct.Pin = GPIO_PIN_0|GPIO_PIN_10;
  GPIO_InitStruct.Mode = GPIO_MODE_OUTPUT_PP;
  GPIO_InitStruct.Pull = GPIO_NOPULL;
  GPIO_InitStruct.Speed = GPIO_SPEED_FREQ_LOW;
  HAL_GPIO_Init(GPIOB, &GPIO_InitStruct);
 
}
 
/* USER CODE BEGIN 4 */
 
/* USER CODE END 4 */
 
/**
  * @brief  This function is executed in case of error occurrence.
  * @retval None
  */
void Error_Handler(void)
{
  /* USER CODE BEGIN Error_Handler_Debug */
  /* User can add his own implementation to report the HAL error return state */
 
  /* USER CODE END Error_Handler_Debug */
}
 
#ifdef  USE_FULL_ASSERT
/**
  * @brief  Reports the name of the source file and the source line number
  *         where the assert_param error has occurred.
  * @param  file: pointer to the source file name
  * @param  line: assert_param error line source number
  * @retval None
  */
void assert_failed(uint8_t *file, uint32_t line)
{ 
  /* USER CODE BEGIN 6 */
  /* User can add his own implementation to report the file name and line number,
     tex: printf("Wrong parameters value: file %s on line %d\r\n", file, line) */
  /* USER CODE END 6 */
}
#endif /* USE_FULL_ASSERT */
 
/************************ (C) COPYRIGHT STMicroelectronics *****END OF FILE****/

[TDK]

You're modifying app_flags within the main thread and within interrupts. Eventually, this will backfire.

The |= operator is not atomic. It first reads the value into a local register, then modifies it, then writes it back to memory. If your interrupt is called between reading and writing, the value of the variable will be overwritten.

You can do the following instead to make the access thread-safe.

__disable_irq();
app_flags |= SOME_APP_FLAG;
__enable_irq();

You could also use bit-banded memory to make access atomic, but that is usually more complicated than it's worth.

[waclawek.jan]

> You could also use bit-banded memory

Not in 'F0/'L0 the OP is using.

Maybe for a couple of flags it's worth to waste a byte per flag.

JW