Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

readRAM has wrong return type #65

Open
Mark-Wills opened this issue Jun 11, 2023 · 0 comments
Open

readRAM has wrong return type #65

Mark-Wills opened this issue Jun 11, 2023 · 0 comments
Assignees
Labels

Comments

@Mark-Wills
Copy link

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

  • Library Version: 1.2.0 (latest available from PlatformIO)
  • IDE Version:
  • Platform: ESP32
  • Subsystem:

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.

@Mark-Wills Mark-Wills added the bug label Jun 11, 2023
SV-Zanshin added a commit that referenced this issue Jun 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants