Zanduino / MCP7940

Arduino Library to access the MCP7940M, MCP7940N and MCP7940x Real-Time chips
GNU General Public License v3.0
35 stars 22 forks source link

readRAM has wrong return type #65

Open Mark-Wills opened 1 year ago

Mark-Wills commented 1 year ago

Expected Behavior

According to function header notes, the return type is a "Pointer to return data structure". This does not make sense as there is no data structure returned. The method behaviour is to populate the variable that is passed in by reference. E.g.:

uint32_t someValue;
rtc.readRam(0, someValue);

Upon execution, someValue contains the value read from RTC RAM.

Actual Behavior

Instead of returning a "pointer to return data structure", the function actually returns the number of bytes read by I2C_READ. However, I believe this is also incorrect: The return type of readRAM is defined as uint8_t&. This is a problem because it is returning a reference to the local variable i, which immediately goes out of scope, resulting in potentially undefined behaviour (generating appropriate compiler warnnings).

Steps to Reproduce the Problem

MCP7940 rtc;
rtc.begin();
uint32_t contents, temp;
temp = rtc.ramRead(0, contents);

Example Compiler Warning

src/rtc.cpp:38:51:   required from here
.pio/libdeps/esp-wrover-kit/MCP7940/src/MCP7940.h:284:13: warning: reference to local variable 'i' returned [-Wreturn-local-addr]
     uint8_t i = I2C_read((addr % 64) + MCP7940_RAM_ADDRESS, value);

Specifications

Comment

I believe the correct implementation would be:

  template <typename T>
  uint8_t readRAM(const uint8_t& addr, T& value) const {
    /*!
     @brief     Template for readRAM()
     @details   As a template it can support compile-time data type definitions
     @param[in] addr Memory address
     @param[in] value    Data Type "T" to read
     @return    Pointer to return data structure
    */
    return I2C_read((addr % 64) + MCP7940_RAM_ADDRESS, value);
  }  // of method readRAM()

This simply returns the number of bytes read, rather than a reference to the number of bytes read.