From ddb98abf1d0403fc402a8b31a864bdb892f4158b Mon Sep 17 00:00:00 2001 From: TedObrien Date: Fri, 28 Mar 2025 21:34:48 +0000 Subject: [PATCH] MCP9808: refactor driver based on feedback - Remove enum class Register : uint8_t - Explicitly initialize buffer - Explicitly initialize temp_raw - Rename uorb publisher - Remove overide from print_status() - Take timestamp after read, correct measurement_time to uint64 and use hrt_elapsed_time() - Remove exit_and_cleanup - Move functions out of main + cleanup whitespace MCP9808: remove exit_and_cleanup MCP9808: Take timestamp after read, correct measurement_time to uint64 and use hrt_elapsed_time() MCP9808: remove overide from print_status() MCP9808: rename uorb publisher MCP9808: explicitly initialize temp_raw MCP9808: explicitly initialize buffer MCP9808: Remove enum class Register : uint8_t MCP9808: move functions out of main + cleanup whitespace --- .../temperature_sensor/mcp9808/mcp9808.cpp | 64 +++++++++++++++---- .../temperature_sensor/mcp9808/mcp9808.h | 31 ++++----- .../mcp9808/mcp9808_main.cpp | 55 ---------------- 3 files changed, 64 insertions(+), 86 deletions(-) diff --git a/src/drivers/temperature_sensor/mcp9808/mcp9808.cpp b/src/drivers/temperature_sensor/mcp9808/mcp9808.cpp index 2cfebda67d..f211510fdb 100644 --- a/src/drivers/temperature_sensor/mcp9808/mcp9808.cpp +++ b/src/drivers/temperature_sensor/mcp9808/mcp9808.cpp @@ -33,16 +33,58 @@ #include "mcp9808.h" +MCP9808::MCP9808(const I2CSPIDriverConfig &config) : + I2C(config), + I2CSPIDriver(config), + _cycle_perf(perf_alloc(PC_ELAPSED, MODULE_NAME": single-sample")), + _comms_errors(perf_alloc(PC_COUNT, MODULE_NAME": comms errors")) +{ + _sensor_temp.device_id = this->get_device_id(); +} + +MCP9808::~MCP9808() +{ + ScheduleClear(); + perf_free(_cycle_perf); + perf_free(_comms_errors); +} + +void MCP9808::RunImpl() +{ + perf_begin(_cycle_perf); + + // publish at around 5HZ + if ((hrt_elapsed_time(&measurement_time)) > 200_ms) { + + float temperature = read_temperature(); + measurement_time = hrt_absolute_time(); // get the time the measurement was taken + + if (std::isnan(temperature)) { + + perf_count(_comms_errors); + + } else { + _sensor_temp.timestamp = hrt_absolute_time(); + _sensor_temp.timestamp_sample = measurement_time; + _sensor_temp.temperature = temperature; + + _sensor_temp_pub.publish(_sensor_temp); + } + } + + perf_end(_cycle_perf); +} + int MCP9808::probe() { uint16_t manuf_id, device_id; - int ret = read_reg(Register::MANUF_ID, manuf_id); + int ret = read_reg(MCP9808_REG_MANUF_ID, manuf_id); if (ret != PX4_OK) { return ret; } - ret = read_reg(Register::DEVICE_ID, device_id); + ret = read_reg(MCP9808_REG_DEVICE_ID, device_id); if (ret != PX4_OK) { return ret; @@ -69,14 +111,14 @@ int MCP9808::init() PX4_INFO("I2C initialized successfully"); - ret = write_reg(Register::CONFIG, 0x0000); // Ensure default configuration + ret = write_reg(MCP9808_REG_CONFIG, 0x0000); // Ensure default configuration if (ret != PX4_OK) { PX4_ERR("Configuration failed"); return ret; } - _to_sensor_temp.advertise(); + _sensor_temp_pub.advertise(); ScheduleOnInterval(100_ms); // Sample at 10 Hz return PX4_OK; @@ -84,9 +126,9 @@ int MCP9808::init() float MCP9808::read_temperature() { - uint16_t temp_raw; + uint16_t temp_raw = 0; - if (read_reg(Register::AMBIENT_TEMP, temp_raw) != PX4_OK) { + if (read_reg(MCP9808_REG_AMBIENT_TEMP, temp_raw) != PX4_OK) { return NAN; } @@ -99,10 +141,10 @@ float MCP9808::read_temperature() return temp; } -int MCP9808::read_reg(Register address, uint16_t &data) +int MCP9808::read_reg(uint8_t address, uint16_t &data) { - uint8_t addr = static_cast(address); - uint8_t buffer[2]; // Buffer to hold the raw data + uint8_t addr = address; + uint8_t buffer[2] = {}; // Buffer to hold the raw data // Read 2 bytes from the register int ret = transfer(&addr, 1, buffer, 2); @@ -117,8 +159,8 @@ int MCP9808::read_reg(Register address, uint16_t &data) return PX4_OK; } -int MCP9808::write_reg(Register address, uint16_t value) +int MCP9808::write_reg(uint8_t address, uint16_t value) { - uint8_t buf[3] = {static_cast(address), static_cast(value >> 8), static_cast(value & 0xFF)}; + uint8_t buf[3] = {address, static_cast(value >> 8), static_cast(value & 0xFF)}; return transfer(buf, sizeof(buf), nullptr, 0); } diff --git a/src/drivers/temperature_sensor/mcp9808/mcp9808.h b/src/drivers/temperature_sensor/mcp9808/mcp9808.h index 907b07db77..ef2e561d7c 100644 --- a/src/drivers/temperature_sensor/mcp9808/mcp9808.h +++ b/src/drivers/temperature_sensor/mcp9808/mcp9808.h @@ -44,6 +44,12 @@ using namespace time_literals; +#define MCP9808_REG_CONFIG 0x01 +#define MCP9808_REG_AMBIENT_TEMP 0x05 +#define MCP9808_REG_MANUF_ID 0x06 +#define MCP9808_REG_DEVICE_ID 0x07 +#define MCP9808_REG_RESOLUTION 0x08 + class MCP9808 : public device::I2C, public I2CSPIDriver { public: @@ -56,30 +62,15 @@ public: static void print_usage(); protected: - void print_status() override; - void exit_and_cleanup() override; + void print_status(); private: - enum class Register : uint8_t { - CONFIG = 0x01, - AMBIENT_TEMP = 0x05, - MANUF_ID = 0x06, - DEVICE_ID = 0x07, - RESOLUTION = 0x08 - }; - - uORB::PublicationMulti _to_sensor_temp{ORB_ID(sensor_temp)}; + uORB::PublicationMulti _sensor_temp_pub{ORB_ID(sensor_temp)}; perf_counter_t _cycle_perf; perf_counter_t _comms_errors; - - sensor_temp_s _sensor_temp{}; - - - int read_reg(Register address, uint16_t &data); - int write_reg(Register address, uint16_t value); + int read_reg(uint8_t address, uint16_t &data); + int write_reg(uint8_t address, uint16_t value); float read_temperature(); - - uint32_t measurement_time = 0; - + hrt_abstime measurement_time = 0; }; diff --git a/src/drivers/temperature_sensor/mcp9808/mcp9808_main.cpp b/src/drivers/temperature_sensor/mcp9808/mcp9808_main.cpp index d58a80001e..445a94a433 100644 --- a/src/drivers/temperature_sensor/mcp9808/mcp9808_main.cpp +++ b/src/drivers/temperature_sensor/mcp9808/mcp9808_main.cpp @@ -41,59 +41,6 @@ #include "mcp9808.h" #include -MCP9808::MCP9808(const I2CSPIDriverConfig &config) : - I2C(config), - I2CSPIDriver(config), - _cycle_perf(perf_alloc(PC_ELAPSED, MODULE_NAME": single-sample")), - _comms_errors(perf_alloc(PC_COUNT, MODULE_NAME": comms errors")) - -{ - _sensor_temp.device_id = this->get_device_id(); - -} - -MCP9808::~MCP9808() -{ - ScheduleClear(); - perf_free(_cycle_perf); - perf_free(_comms_errors); - -} - -void MCP9808::exit_and_cleanup() -{ - I2CSPIDriverBase::exit_and_cleanup(); // nothing to do -} - -void MCP9808::RunImpl() -{ - perf_begin(_cycle_perf); - - // publish at around 5HZ - if ((hrt_absolute_time() - measurement_time) > 200_ms) { - - measurement_time = hrt_absolute_time(); // get the time the measurement was taken - float temperature = read_temperature(); - - if (std::isnan(temperature)) { - - perf_count(_comms_errors); - - } else { - - _sensor_temp.timestamp = hrt_absolute_time(); - _sensor_temp.timestamp_sample = measurement_time; - _sensor_temp.temperature = temperature; - - _to_sensor_temp.publish(_sensor_temp); - } - - - } - - perf_end(_cycle_perf); -} - void MCP9808::print_usage() { PRINT_MODULE_USAGE_NAME("mcp9808", "driver"); @@ -103,13 +50,11 @@ void MCP9808::print_usage() PRINT_MODULE_USAGE_DEFAULT_COMMANDS(); } - void MCP9808::print_status() { I2CSPIDriverBase::print_status(); perf_print_counter(_cycle_perf); perf_print_counter(_comms_errors); - } extern "C" int mcp9808_main(int argc, char *argv[])