feat: enhance error handling and add device error reporting for radio operations

CMakeLists.txt

@@ -9,11 +9,11 @@ idf_component_register(
                 app_constant
 )
 
-# This option would make LLCC68 ignore `SPI_CMD_INVALID` error.
+option(APP_SPI_DISABLE_INVALID_STATUS_CHECK "make driver ignore `SPI_CMD_INVALID` error" OFF)
-option(APP_SPI_DISABLE_INVALID_STATUS_CHECK "Disable invalid status check" OFF)
 if (APP_SPI_DISABLE_INVALID_STATUS_CHECK)
         target_compile_definitions(${COMPONENT_LIB} PUBLIC APP_SPI_DISABLE_INVALID_STATUS_CHECK)
 endif()
+
 option(APP_RADIO_DISABLE_CALIBRATION "Disable radio calibration" ON)
 if (APP_RADIO_DISABLE_CALIBRATION)
         target_compile_definitions(${COMPONENT_LIB} PUBLIC APP_RADIO_DISABLE_CALIBRATION)

inc/hal_error.hpp

@@ -32,17 +32,18 @@ constexpr t GENERIC_ERR_BASE = 0x120;
 constexpr t AGAIN            = GENERIC_ERR_BASE + 1; /*!< Operation failed, retry */
 constexpr t BUSY             = GENERIC_ERR_BASE + 2; /*!< Busy */
 
-constexpr t SPI_ERR_BASE = 0x1'2000;
+constexpr t RADIO_TRANS_ERR_BASE = 0x1'2000;
 // A transaction from host took too long to complete and triggered an internal watchdog.
 // The watchdog mechanism can be disabled by host; it is meant to ensure all outcomes are flagged to the host MCU.
-constexpr t SPI_TIMEOUT = SPI_ERR_BASE + 2;
+constexpr t RADIO_TRANS_TIMEOUT = RADIO_TRANS_ERR_BASE + 2;
 // Processor was unable to process command either because of an invalid opcode or
 // because an incorrect number of parameters has been provided.
-constexpr t SPI_CMD_INVALID = SPI_ERR_BASE + 3;
+constexpr t RADIO_TRANS_CMD_PROC_ERR = RADIO_TRANS_ERR_BASE + 3;
 // The command was successfully processed, however the chip could not execute the command;
 // for instance it was unable to enter the specified device mode or send the requested data
-constexpr t SPI_CMD_FAILED          = SPI_ERR_BASE + 4;
+constexpr t RADIO_TRANS_FAIL_TO_EXE = RADIO_TRANS_ERR_BASE + 4;
-constexpr t SPI_INVALID_RADIO_STATE = SPI_ERR_BASE + 5; /*!< Radio is in an invalid state for the requested operation */
+/*!< Radio is in an expected state (not necessary an error) */
+constexpr t RADIO_TRANS_INVALID_RADIO_STATE = RADIO_TRANS_ERR_BASE + 5;
 
 constexpr t RADIO_ERR_BASE                 = 0x1'3000;
 constexpr t RADIO_CHIP_NOT_FOUND           = RADIO_ERR_BASE + 1;
@@ -75,10 +76,10 @@ constexpr auto error_table = std::to_array<std::tuple<t, const char *>>(
 		APP_ERR_TBL_IT(AGAIN),
 		APP_ERR_TBL_IT(BUSY),
 
-		APP_ERR_TBL_IT(SPI_TIMEOUT),
+		APP_ERR_TBL_IT(RADIO_TRANS_TIMEOUT),
-		APP_ERR_TBL_IT(SPI_CMD_INVALID),
+		APP_ERR_TBL_IT(RADIO_TRANS_CMD_PROC_ERR),
-		APP_ERR_TBL_IT(SPI_CMD_FAILED),
+		APP_ERR_TBL_IT(RADIO_TRANS_FAIL_TO_EXE),
-		APP_ERR_TBL_IT(SPI_INVALID_RADIO_STATE),
+		APP_ERR_TBL_IT(RADIO_TRANS_INVALID_RADIO_STATE),
 
 		APP_ERR_TBL_IT(RADIO_CHIP_NOT_FOUND),
 		APP_ERR_TBL_IT(RADIO_INVALID_TCXO_VOLTAGE),

inc/hal_spi.hpp

@@ -51,14 +51,14 @@ inline error_t status_to_err(const uint8_t status) {
 	error_t status_ok = [](llcc68::CommandStatus st) {
 		switch (st) {
 		case llcc68::CommandStatus::COMMAND_TIMEOUT:
-			return error::SPI_TIMEOUT;
+			return error::RADIO_TRANS_TIMEOUT;
 		case llcc68::CommandStatus::FAILURE_TO_EXECUTE_COMMAND:
-			return error::SPI_CMD_FAILED;
+			return error::RADIO_TRANS_FAIL_TO_EXE;
 		case llcc68::CommandStatus::COMMAND_PROCESSING_ERROR:
 #ifdef APP_SPI_DISABLE_INVALID_STATUS_CHECK
 			return error::OK;
 #else
-			return error::SPI_CMD_INVALID;
+			return error::RADIO_TRANS_CMD_PROC_ERR;
 #endif
 		default:
 			return error::OK;
@@ -74,7 +74,7 @@ inline error_t status_to_err(const uint8_t status) {
 		case llcc68::ChipMode::STBY_RC:
 			return error::OK;
 		default:
-			return error::SPI_INVALID_RADIO_STATE;
+			return error::RADIO_TRANS_INVALID_RADIO_STATE;
 		}
 	}(st.chip_mode);
 	return chip_mode_usual;

inc/llcc68.hpp

@@ -255,24 +255,26 @@ get_packet_status() {
 		.raw = {data[0], data[1], data[2]}};
 }
 
-inline Result<op_error_t, error_t>
+/**
+ * @note ignore any possible transmission errors
+ */
+inline Result<DeviceErrors, error_t>
 get_device_errors() {
-	uint8_t data[] = {0x00, 0x00};
+	uint8_t data[]           = {0x00, 0x00};
-	const auto res = spi::read_stream(RADIOLIB_SX126X_CMD_GET_DEVICE_ERRORS, data);
+	std::ignore              = spi::read_stream(RADIOLIB_SX126X_CMD_GET_DEVICE_ERRORS, data);
-	APP_RADIO_RETURN_ERR(res);
+	uint16_t device_err_code = data[0] << 8 | data[1];
-	// assuming little endian (as most microcontrollers are)
+	return *reinterpret_cast<DeviceErrors *>(&device_err_code);
-	uint16_t opError_le = data[0] << 8 | data[1];
-	auto opError        = *reinterpret_cast<op_error_t *>(&opError_le);
-	return opError;
 }
 
 /**
  * \brief This commands clears all the errors recorded in the device. The errors can not be cleared independently.
+ * \note ignore any possible transmission errors
  */
 inline Result<Unit, error_t>
 clear_device_errors() {
 	constexpr uint8_t data[] = {RADIOLIB_SX126X_CMD_NOP, RADIOLIB_SX126X_CMD_NOP};
-	return spi::write_stream(RADIOLIB_SX126X_CMD_CLEAR_DEVICE_ERRORS, data);
+	std::ignore              = spi::write_stream(RADIOLIB_SX126X_CMD_CLEAR_DEVICE_ERRORS, data);
+	return {};
 }
 
 /**
@@ -296,7 +298,7 @@ inline constexpr uint32_t frequency_raw(const freq_t freq) {
 inline Result<Unit, error_t>
 set_frequency(freq_t freq, const bool calibrate = true) {
 	using f_t = decltype(freq);
-	if (!valid_freq(freq)) {
+	if (not valid_freq(freq)) {
 		return ue_t{error_t{error::RADIO_INVALID_FREQUENCY}};
 	}
 	if (calibrate) {
@@ -322,13 +324,7 @@ set_frequency(freq_t freq, const bool calibrate = true) {
 		delay_ms(5);
 	}
 
-	static freq_t last_freq       = 0;
+	return set_rf_frequency(frequency_raw(freq));
-	static uint32_t raw_last_freq = 0;
-	if (last_freq != freq) {
-		last_freq     = freq;
-		raw_last_freq = frequency_raw(freq);
-	}
-	return set_rf_frequency(raw_last_freq);
 }
 
 inline Result<uint8_t, error_t>
@@ -1232,6 +1228,19 @@ static constexpr auto init_pins = [](void (*isr)(void *), void *arg) {
 	init_tx_rx_en();
 };
 
+inline Result<Unit, error_t> get_device_error_print_and_clear(const char *tag) {
+	auto res = get_device_errors();
+	if (not res) {
+		return ue_t{res.error()};
+	}
+	if (res->has_any_error()) {
+		ESP_LOGE(tag, "%s", res->to_string().c_str());
+	} else {
+		ESP_LOGI(tag, "no device errors");
+	}
+	return clear_device_errors();
+}
+
 static constexpr auto begin = [](const lora_parameters_t &params) -> Result<Unit, error_t> {
 	/**
 	 * Most of the commands can be sent in any order except for the radio configuration commands which will set the radio in
@@ -1297,6 +1306,7 @@ static constexpr auto begin = [](const lora_parameters_t &params) -> Result<Unit
 								mod_params.bw,
 								mod_params.cr,
 								mod_params.ldr_optimize);
+	res = get_device_error_print_and_clear(TAG);
 	APP_RADIO_RETURN_ERR_CTX(res, "set modulation params");
 
 	res = set_lora_sync_word(params.sync_word);
@@ -1313,8 +1323,10 @@ static constexpr auto begin = [](const lora_parameters_t &params) -> Result<Unit
 
 #ifdef APP_RADIO_DISABLE_CALIBRATION
 	res = set_frequency(params.frequency, false);
+	res = get_device_error_print_and_clear(TAG);
 #else
 	res = set_frequency(params.frequency, true);
+	res = get_device_error_print_and_clear(TAG);
 #endif /* APP_RADIO_DISABLE_CALIBRATION */
 
 	APP_RADIO_RETURN_ERR_CTX(res, "set frequency");

inc/llcc68_definitions.hpp

@@ -6,6 +6,7 @@
 #define LLCC68_DEFINITIONS_H
 #include <cstdint>
 #include <optional>
+#include <sstream>
 #include <string>
 #include <format>
 #include <tuple>
@@ -348,20 +349,60 @@ union packet_status_t {
 };
 
 
-struct __attribute__((packed)) op_error_t {
+struct __attribute__((packed)) DeviceErrors {
-	bool RC64K_CALIB_ERR : 1; // RC64k calibration failed
+	// LSB
-	bool RC13M_CALIB_ERR : 1; // RC13M calibration failed
-	bool PLL_CALIB_ERR : 1;   // PLL calibration failed
-	bool ADC_CALIB_ERR : 1;   // ADC calibration failed
-	bool IMG_CALIB_ERR : 1;   // IMG calibration failed
-	bool XOSC_START_ERR : 1;  // XOSC failed to start
-	bool PLL_LOCK_ERR : 1;    // PLL failed to lock
-	uint8_t rfu_1 : 1;        // RFU
-	bool PA_RAMP_ERR : 1;     // PA ramping failed
-	uint8_t rfu_2 : 7;        // RFU
-};
 
-static_assert(sizeof(op_error_t) == 2);
+	bool RC64K_CALIB_ERR : 1; // [0] RC64k calibration failed
+	bool RC13M_CALIB_ERR : 1; // [1] RkC13M calibration failed
+	bool PLL_CALIB_ERR : 1;   // [2] PLL calibration failed
+	bool ADC_CALIB_ERR : 1;   // [3] ADC calibration failed
+	bool IMG_CALIB_ERR : 1;   // [4] IMG calibration failed
+	bool XOSC_START_ERR : 1;  // [5] XOSC failed to start
+	bool PLL_LOCK_ERR : 1;    // [6] PLL failed to lock
+	uint8_t rfu_1 : 1;        // [7] RFU
+	bool PA_RAMP_ERR : 1;     // [8] PA ramping failed
+	uint8_t rfu_2 : 7;        // [15:9] RFU
+
+	// MSB
+
+	std::string to_string() const {
+		auto ss = std::stringstream();
+		ss << "DeviceErrors{";
+		if (RC64K_CALIB_ERR) {
+			ss << "RC64K_CALIB_ERR(0) ";
+		}
+		if (RC13M_CALIB_ERR) {
+			ss << "RC13M_CALIB_ERR(1) ";
+		}
+		if (PLL_CALIB_ERR) {
+			ss << "PLL_CALIB_ERR(2) ";
+		}
+		if (ADC_CALIB_ERR) {
+			ss << "ADC_CALIB_ERR(3) ";
+		}
+		if (IMG_CALIB_ERR) {
+			ss << "IMG_CALIB_ERR(4) ";
+		}
+		if (XOSC_START_ERR) {
+			ss << "XOSC_START_ERR(5) ";
+		}
+		if (PLL_LOCK_ERR) {
+			ss << "PLL_LOCK_ERR(6) ";
+		}
+		if (PA_RAMP_ERR) {
+			ss << "PA_RAMP_ERR(8) ";
+		}
+		ss << "}";
+		return ss.str();
+	}
+
+	[[nodiscard]]
+	bool has_any_error() const {
+		return RC64K_CALIB_ERR || RC13M_CALIB_ERR || PLL_CALIB_ERR || ADC_CALIB_ERR ||
+			   IMG_CALIB_ERR || XOSC_START_ERR || PLL_LOCK_ERR || PA_RAMP_ERR;
+	}
+};
+static_assert(sizeof(DeviceErrors) == 2);
 
 constexpr bool in_range(const auto v, const auto min, const auto max) {
 	return v >= min && v <= max;

src/hal_spi.cpp

@@ -196,12 +196,12 @@ read_stream(uint8_t cmd, std::span<uint8_t> data, const size_t timeout_ms) {
 		return ue_t{error::FAILED};
 	}
 
+	std::copy(rx_buffer.begin() + 1, rx_buffer.end(), data.begin());
+
 	auto status = rx_buffer[0];
 	if (const auto err = status_to_err(status); err != error::OK) {
 		return ue_t{err};
 	}
-
-	std::copy(rx_buffer.begin() + 1, rx_buffer.end(), data.begin());
 	return {};
 }