Skip to content

feat(pcf85063): Add espp::Pcf85063 peripheral for PCF85063 RTC #476

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

Merged
merged 2 commits into from
Jul 14, 2025

Conversation

finger563
Copy link
Contributor

@finger563 finger563 commented Jul 14, 2025

Description

  • Add PCF85063 component for communicating with the I2C RTC
  • Update BM8563 RTC component to move bcd related conversion functions to utils component and update deps accordingly
  • Update ws-s3-touch component to have rtc related code
  • Update ws-s3-touch/example to test rtc

Motivation and Context

RTCs are nice, and the Waveshare S3 TouchLCD has this RTC, so supporting it would be nice.

How has this been tested?

  • Build and run pcf85063/example on Waveshare S3 TouchLCD
  • BUild and run ws-s3-touch/example on Waveshare S3 TouchLCD

Screenshots (if appropriate, e.g. schematic, board, console logs, lab pictures):

CleanShot 2025-07-13 at 22 14 08

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation Update
  • Hardware (schematic, board, system design) change
  • Software change

Checklist:

  • My change requires a change to the documentation.
  • I have added / updated the documentation related to this change via either README or WIKI

Software

  • I have added tests to cover my changes.
  • I have updated the .github/workflows/build.yml file to add my new test to the automated cloud build github action.
  • All new and existing tests passed.
  • My code follows the code style of this project.

@finger563 finger563 self-assigned this Jul 14, 2025
@finger563 finger563 requested a review from Copilot July 14, 2025 03:29
@finger563 finger563 added enhancement New feature or request rtc real time clock waveshare s3 touch lcd pcf85063 bm8563 real time clock peripheral driver utils labels Jul 14, 2025
Copy link

✅Static analysis result - no issues found! ✅

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR introduces the new espp::Pcf85063 RTC driver, integrates it into the Waveshare S3 TouchLCD component, and updates documentation and CI to cover the new peripheral.

  • Add PCF85063 driver component with full API, example, and IDF manifest
  • Refactor BCD conversion into utils, update BM8563 to use shared conversions
  • Integrate RTC support into ws-s3-touch component and its example, update documentation and workflows

Reviewed Changes

Copilot reviewed 26 out of 26 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
doc/en/rtc/pcf85063_example.md Include the example README for PCF85063
doc/en/rtc/pcf85063.rst Add PCF85063 documentation and example to the toctree
doc/en/rtc/index.rst Register PCF85063 in the RTC index
doc/Doxyfile Add PCF85063 example and header to Doxygen paths
components/ws-s3-touch/src/rtc.cpp Implement RTC initialization and accessor in WsS3Touch
components/ws-s3-touch/include/ws-s3-touch.hpp Add Rtc alias, methods, and member for PCF85063
components/ws-s3-touch/idf_component.yml Declare dependency on espp/pcf85063
components/ws-s3-touch/example/main/ws_s3_touch_example.cpp Initialize RTC, update UI label, and add RTC task in example
components/ws-s3-touch/CMakeLists.txt Add pcf85063 to component requirements
components/utils/include/espp_chrono.hpp Introduce BCD conversion utilities
components/pcf85063/include/pcf85063.hpp New PCF85063 driver implementation
components/pcf85063/idf_component.yml IDF manifest for PCF85063 component
components/pcf85063/example/**/* Example, CMake, README, and Kconfig for PCF85063 component
.github/workflows/upload_components.yml Include PCF85063 in upload matrix
.github/workflows/build.yml Add PCF85063 example to build matrix
Comments suppressed due to low confidence (2)

components/utils/include/espp_chrono.hpp:1

  • [nitpick] The file espp_chrono.hpp provides only BCD conversion functions, not chrono utilities. For clarity and maintainability, consider renaming it (e.g., espp_bcd.hpp) to better reflect its functionality.
#pragma once

.github/workflows/build.yml:146

  • The CI build matrix now runs the PCF85063 example but doesn’t include the ws-s3-touch example. To ensure integration coverage, add an entry for components/ws-s3-touch/example.
          target: esp32s3

.write = std::bind_front(&espp::I2c::write, &internal_i2c_),
.read = std::bind_front(&espp::I2c::read, &internal_i2c_),
.log_level = espp::Logger::Verbosity::WARN,
});
Copy link
Preview

Copilot AI Jul 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

initialize_rtc() always returns true, even if the underlying Pcf85063 initialization fails. Consider capturing the error from the Rtc constructor or its init method and returning false on failure to allow callers to handle initialization errors properly.

Suggested change
});
});
if (!rtc_ || !rtc_->init()) {
logger_.error("Failed to initialize RTC");
return false;
}

Copilot uses AI. Check for mistakes.

Comment on lines +260 to +264
std::error_code ec;
std::tm timeinfo = rtc->get_time(ec);
if (ec) {
logger.error("Failed to get RTC time: {}", ec.message());
} else {
Copy link
Preview

Copilot AI Jul 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] In the RTC task, on failure to get the time you log the error but continue running indefinitely, which can spam logs. Consider returning true to stop the task after repeated errors or adding retry/backoff logic.

Suggested change
std::error_code ec;
std::tm timeinfo = rtc->get_time(ec);
if (ec) {
logger.error("Failed to get RTC time: {}", ec.message());
} else {
static int failure_count = 0; // Track consecutive failures
constexpr int max_failures = 5; // Maximum allowed failures
std::error_code ec;
std::tm timeinfo = rtc->get_time(ec);
if (ec) {
failure_count++;
logger.error("Failed to get RTC time (attempt {}): {}", failure_count, ec.message());
if (failure_count >= max_failures) {
logger.critical("Maximum RTC retrieval failures reached. Terminating task.");
return true; // Stop the task
}
} else {
failure_count = 0; // Reset failure count on success

Copilot uses AI. Check for mistakes.

@finger563 finger563 merged commit ea2642f into main Jul 14, 2025
88 checks passed
@finger563 finger563 deleted the feat/pcf85063-rtc branch July 14, 2025 03:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bm8563 real time clock peripheral driver enhancement New feature or request pcf85063 rtc real time clock utils waveshare s3 touch lcd
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant