m5stack / M5Core2

M5Core2 Arduino Library
MIT License
267 stars 117 forks source link

Strange comparison in RTC code #122

Closed supcik closed 1 year ago

supcik commented 1 year ago

Hello M5Stack.

I'm using your code with ESP-IDF and the compiler gave me the warning : "comparison is always true" at the following location :

https://github.com/m5stack/M5Core2/blob/f9589994bfc432119a9121f3f42f8a1ade7105f9/src/RTC.cpp#L221

Indeed, the types of the fields Minutes, Hours, ... of RTC_TimeTypeDef are all uint8_t and an unsigned integer is always positive. So the first 4 tests in the SetAlarmIRQ method seem to be useless, and the variable irq_enable is thus also not needed. Or did I miss something?

Please tell me if you want me to submit a pull request with the simplified version (without the tests).

Tinyu-Zhao commented 1 year ago

Thanks for catching the problem, it might be caused by some oversight, you can try to post a PR to help us optimize it.

JM-FRANCE commented 1 year ago

well since all the attributes are always >= 0 the optimiser removes the test so it's just as if the code was

int RTC::SetAlarmIRQ(const RTC_TimeTypeDef &RTC_TimeStruct) {
  uint8_t out_buf[4] = {0x80, 0x80, 0x80, 0x80};
  out_buf[0] = ByteToBcd2(RTC_TimeStruct.Minutes) & 0x7f;
  out_buf[1] = ByteToBcd2(RTC_TimeStruct.Hours) & 0x3f;
  uint8_t reg_value = ReadReg(0x01);
  reg_value |= (1 << 1);
  for (int i = 0; i < sizeof out_buf; i++) WriteReg(0x09 + i, out_buf[i]);
  WriteReg(0x01, reg_value);
  return 1;
}

int RTC::SetAlarmIRQ(const RTC_DateTypeDef &RTC_DateStruct,
                     const RTC_TimeTypeDef &RTC_TimeStruct) {
  uint8_t out_buf[4] = {0x80, 0x80, 0x80, 0x80};
  out_buf[0] = ByteToBcd2(RTC_TimeStruct.Minutes) & 0x7f;
  out_buf[1] = ByteToBcd2(RTC_TimeStruct.Hours) & 0x3f;
  out_buf[2] = ByteToBcd2(RTC_DateStruct.Date) & 0x3f;
  out_buf[3] = ByteToBcd2(RTC_DateStruct.WeekDay) & 0x07;
  uint8_t reg_value = ReadReg(0x01);
  reg_value |= (1 << 1);
  for (int i = 0; i < sizeof out_buf; i++) WriteReg(0x09 + i, out_buf[i]);
  WriteReg(0x01, reg_value);
  return 1;
}

using that code would remove the warnings

I did not test it !

Tinyu-Zhao commented 1 year ago

Fixed. https://github.com/m5stack/M5Core2/commit/7938c7ee42277fede8c0dee39e0b906bb7ee87e7