MRaninecADI
commented
4 years ago Hi Mihail,
Thanks for your feedback. I addressed all the points you highlighted below and pushed the changes to the cn04289_v2 branch. Is there anything else you need me to do before we can publish this?
Best Regards, Michal
From: Mihail Chindris notifications@github.com Sent: Friday, March 20, 2020 14:19 To: analogdevicesinc/EVAL-ADICUP3029 EVAL-ADICUP3029@noreply.github.com Cc: Raninec, Michal Michal.Raninec@analog.com; Assign assign@noreply.github.com Subject: Re: [analogdevicesinc/EVAL-ADICUP3029] Cn04289 v2 (#47)
[External]
@mchindri commented on this pull request.
In projects/ADuCM3029_demo_cn0428_cn0429_v2/src/adi_m355_gas_sensor.cpphttps://github.com/analogdevicesinc/EVAL-ADICUP3029/pull/47#discussion_r395585940:
+{
+
+/*
-
- Open M355 gas sensor
-
*/
+SENSOR_RESULT M355_GAS::open()
+{
-
pADI_GPIO0->DS |= (1 << 4) | (1 << 5);
-
this->m_i2c_address = ADI_CFG_I2C_DEFAULT_ADDR;
-
return this->InitI2C();
+}
+
+/*
-
- Open M355 gas sensor with selected address
-
*/
+SENSOR_RESULT M355_GAS::openWithAddr(uint8_t sensor_address)
There is no need for other name for open in c++. You can have both M355_GAS::open() and M355_GAS::open(uint8_t sensor_address) and works fine due to c++ function overloading
In projects/ADuCM3029_demo_cn0428_cn0429_v2/src/adi_gas_sensor.hhttps://github.com/analogdevicesinc/EVAL-ADICUP3029/pull/47#discussion_r395588634:
+
+#ifndef ADI_GAS_SENSORH
+#define ADI_GAS_SENSORH
+
+#include "base_sensor/adi_sensor.h"
+#include "adi_m355_gas_sensor_config.h"
+
+namespace adi_sensor_swpack
+{
+/**
-
- @class Gas_Reading.
-
*
-
- @brief Gas Sensor Class. Basic functions borrowed by water sensor also.
-
*
-
**/
+class Gas_Reading: public Sensor
Wouldn't Gas_Sensor be a better naming than Gas_Reading?
In projects/ADuCM3029_demo_cn0428_cn0429_v2/src/adi_m355_gas_sensor.cpphttps://github.com/analogdevicesinc/EVAL-ADICUP3029/pull/47#discussion_r395646266:
- uint32_t nHwErrors;
-
uint8_t aPrologueData[5u];
-
/ Set slave address for ADI M355 gas sensor /
-
eI2cResult = adi_i2c_SetSlaveAddress(m_i2c_handle, this->m_i2c_address);
-
if (eI2cResult != ADI_I2C_SUCCESS)
-
return SET_SENSOR_ERROR(SENSOR_ERROR_I2C, eI2cResult);
-
if (RW == READ) {
-
aPrologueData[0] = RegAddr;
-
sTransfer.pPrologue = &aPrologueData[0u];
-
sTransfer.nPrologueSize = 1u;
-
sTransfer.pData = pData;
-
sTransfer.nDataSize = size;
-
sTransfer.bReadNotWrite = true;
⬇️ Suggested change
-
sTransfer.bReadNotWrite = true;
-
sTransfer.bReadNotWrite = RW == READ ? true : false;
This way is not needed the if else statement and write almost the same code in each block. Or an other idea is to create a function for read and a function for write.
In projects/ADuCM3029_demo_cn0428_cn0429_v2/src/adi_m355_gas_sensor.cpphttps://github.com/analogdevicesinc/EVAL-ADICUP3029/pull/47#discussion_r395654110:
- pBuff = &new_I2C_address;
+
-
/* IMPORTANT NOTE
-
- IF USING THIS DRIVER, ENSURE ONLY ONE SLAVE CS IS SET LOW BEFORE
-
- CALLING OTHERWISE ADDRESS CONFLICTS COULD HAPPEN
-
*/
-
/ Send command to change I2C address /
-
Result = this->open(); / uses default address /
-
if (Result != SENSOR_ERROR_NONE)
-
return Result;
-
delay_ms(50);
-
Result = this->I2CReadWrite(WRITE, SET_I2C_ADDRESS, pBuff, 1);
-
if (Result != SENSOR_ERROR_NONE)
-
return Result;
-
this->close();
It looks a bit confusing to me the open and close instruction. Wouldn't be better to send the i2c change command inside this function? Also with the 2 calls to open, the I2C device is initialized twice. I think this is not needed.
— You are receiving this because you were assigned. Reply to this email directly, view it on GitHubhttps://github.com/analogdevicesinc/EVAL-ADICUP3029/pull/47#pullrequestreview-378417195, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ALC5L7H6EIZKMFDITAHN633RIN3OXANCNFSM4LPL4VSA.
adrimbarean
This is a branch containing updated version of the CN0428_CN0429 reference design firmware. Please review these changes and merge with master if all good. Thanks!