-
Notifications
You must be signed in to change notification settings - Fork 14
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
Conversation
✅Static analysis result - no issues found! ✅ |
There was a problem hiding this 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 forcomponents/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, | ||
}); |
There was a problem hiding this comment.
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.
}); | |
}); | |
if (!rtc_ || !rtc_->init()) { | |
logger_.error("Failed to initialize RTC"); | |
return false; | |
} |
Copilot uses AI. Check for mistakes.
std::error_code ec; | ||
std::tm timeinfo = rtc->get_time(ec); | ||
if (ec) { | ||
logger.error("Failed to get RTC time: {}", ec.message()); | ||
} else { |
There was a problem hiding this comment.
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.
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.
Description
utils
component and update deps accordinglyws-s3-touch
component to have rtc related codews-s3-touch/example
to test rtcMotivation and Context
RTCs are nice, and the Waveshare S3 TouchLCD has this RTC, so supporting it would be nice.
How has this been tested?
pcf85063/example
on Waveshare S3 TouchLCDws-s3-touch/example
on Waveshare S3 TouchLCDScreenshots (if appropriate, e.g. schematic, board, console logs, lab pictures):
Types of changes
Checklist:
Software
.github/workflows/build.yml
file to add my new test to the automated cloud build github action.