Gambalunga branch 1 (read the PCF8523 offset register) #297
Conversation
Method to calibrate PCF8523 RTC including method to read the offset register-
Added method to read the offset register
There was a problem hiding this comment.
This file is mostly redundant with the comments in the example sketch. For consistency with the other provided examples, the file should be removed, and the information needed to understand the example should be provided, in comments, in the example itself.
There was a problem hiding this comment.
I wondered about this and I did largely cover it in the comments. I will put the comments and the readme together and try to make a succinct comment that covers the usage and remove the readme.
src/RTClib.h
Outdated
| @@ -423,6 +423,10 @@ class RTC_PCF8523 : RTC_I2C { | |||
| void disableCountdownTimer(void); | |||
| void deconfigureAllTimers(void); | |||
| void calibrate(Pcf8523OffsetMode mode, int8_t offset); | |||
| int8_t readOffsetReg(); | |||
| String getOffsetMode(); | |||
There was a problem hiding this comment.
Remove trailing white space.
There was a problem hiding this comment.
Will do :) At least this should be easy.
There was a problem hiding this comment.
GitHub shows that every single line of this file has been modified. This is because the end of lines have been changed from Unix style (LF) to DOS style (CRLF). This should be avoided.
There was a problem hiding this comment.
No idea how that happened and I did wonder why it showed all lines as modified.
I assume the best method will be to delete this file and replace it with a new corrected version?
There was a problem hiding this comment.
I would suggest force-pushing a squash commit. Otherwise you can push a commit that replaces this file with the corrected one.
src/RTC_PCF8523.cpp
Outdated
|
|
||
| /**************************************************************************/ | ||
| /*! | ||
| @brief read the offset register |
There was a problem hiding this comment.
Remove trailing white space.
There was a problem hiding this comment.
Again: easy to fix. Will do
src/RTC_PCF8523.cpp
Outdated
| */ | ||
| /**************************************************************************/ | ||
|
|
||
| int8_t RTC_PCF8523::readOffsetReg() { |
There was a problem hiding this comment.
Return type should be uint8_t: it is just a byte; it does not make sense to interpret it as a signed number.
What is the purpose of this method? The way the data is encoded within the offset register of the RTC is an implementation detail the user should not need to be aware of.
The presence of this method could be justified as a way to read both the offset and the mode at once, provided suitable methods are provided for decoding it, e.g.
static Pcf8523OffsetMode getOffsetMode(uint8_t offset_reg);
static int8_t getOffsetValue(uint8_t offset_reg);In which case the next two methods could be dropped.
There was a problem hiding this comment.
Yes, I see your point and you are quite correct. The byte does not represent a signed interger.
I will see if I can also work it out with your suggestion.
Thanks
There was a problem hiding this comment.
@edgar-bonet
I hope you don't mind me continueing this conversation.
I made some changes by directly editing in Github and that generated a whole lot of PR runs for every time I committed a change. This in turn created a lot of apparent failures.
the latest is:
Run bash ci/doxy_gen_and_deploy.sh src Setting up the script... Cloning into 'RTClib'... Generating Doxygen code documentation... Grabbing default Doxyfile /home/runner/work/RTClib/RTClib/src/RTClib.h:426: warning: return type of member RTC_PCF8523::readOffsetReg is not documented /home/runner/work/RTClib/RTClib/src/RTClib.h:427: warning: return type of member RTC_PCF8523::getOffsetMode is not documented /home/runner/work/RTClib/RTClib/src/RTClib.h:428: warning: return type of member RTC_PCF8523::getOffset is not documented Error: Process completed with exit code 1.
I notice in the suggested code above that you used 'static' as far as I can see this only means that once used in a function the variable value is recorded for possible future use in the same funtion but can not be used elsewhere.
I am not sure whether using static in this mode means that the return value is then able to be used in the call to the function from the example sketch.
The reason that I used String is that I was trying to make the method as transparent as possible to the user by returning the actual text. In my original version (only local) I used a global variable but I realised this was not a good idea,
If I return a value, 0 or 1, or a bool, it would still need interpretation in the sketch (as in Method 1 in the sketch)instead of in the library.
I made two possible methods in the example sketch with the idea that whoever eventually merges the branch can choose to leave both or only one of the methods and edit the code as appropriate.
My motivation was more to put forward a concept rather than, perhaps, the final solution. but in order to carry this forward I am quite happy to be guided by you.
My experience with the PCF8523 RTC is that calibration is invariably required so I think this is an important issue.
There was a problem hiding this comment.
I am replying in the main thread.
src/RTC_PCF8523.cpp
Outdated
| @brief read the offset register and return OffsetMode | ||
| */ | ||
| /**************************************************************************/ | ||
| String RTC_PCF8523::getOffsetMode() { |
There was a problem hiding this comment.
Do not return a String: these objects are expensive to build and compare. Return instead a value of type Pcf8523OffsetMode.
src/RTC_PCF8523.cpp
Outdated
| /**************************************************************************/ | ||
| String RTC_PCF8523::getOffsetMode() { | ||
| String OffsetMode; | ||
| // int8_t OffsetReg = readOffsetReg(); |
There was a problem hiding this comment.
Remove useless comment.
There was a problem hiding this comment.
Just a way of visually separating the two functions but if you think it is unnecessary I will remove it.
There was a problem hiding this comment.
Ah! I see what you mean now. Certainly it should be removed. I left it in the file after I changed the method.
Put it down to my lack of attention.
Thanks
In general terms I will sort all this out on my local PC and then upload the corrected code after I have tested it rather than making one correction at a time.
Edgar |
|
Regarding clang-format: this is a program that runs as part of the CI workflow to ensure that the pull request complies with the library's coding style. This is what triggered the “All checks have failed” message at the bottom of this page. As explained by the end of the README, you can install clang-format on your computer and use it to enforce the coding style. Alternatively, if you click on the “Details” link next to the failed check, you will see the build log, which contains a patch listing the required changes:
Regarding the “arduino-library tag” warning, I guess it is about your fork of this repository. As that repo is likely meant for creating pull requests only, I think the warning is irrelevant and you can safely ignore it. |
Content moved to comments in example sketch.
Updated comments
Added methods to obtain data from the offset register: readOffsetReg(); getOffsetMode(); getOffset();
About the latest CI failureThe error message (actually a warning treated as an error) is: (and similar warnings for the other two methods). This is emitted by Doxygen, the software used to generate the online library documentation. It is complaining that the methods do return something (they are not /**************************************************************************/
/*!
@brief read the offset register
@return the raw value of the register
*/
/**************************************************************************/
uint8_t RTC_PCF8523::readOffsetReg() { /* ... */ }See how this is done in other non- About static methodsThe keyword class RTC_PCF8523 : RTC_I2C {
public:
// ...
uint8_t readOffsetReg();
static Pcf8523OffsetMode getOffsetMode(uint8_t offset_reg);
static int8_t getOffsetValue(uint8_t offset_reg);
};then you could use it like this: uint8_t reg_value = rtc.readOffsetReg();
Pcf8523OffsetMode offset_mode = rtc.getOffsetMode(reg_value);
int8_t offset_value = RTC_PCF8563::getOffsetValue(reg_value);Notice that the second line invokes the I personally find that static conversion methods would be the most logical way to handle decoding the register value. This could, however, be difficult to understand for beginners. I guess it is acceptable, as there is already one public static method in the library, namely About returning a String objectI don't quite get what you mean by “transparent”. As I stated in a comment, In this specific case, the library already defines int8_t offset_value = rtc.getOffset();
float drift_ppm;
if (rtc.getOffsetMode() == PCF8523_TwoHours)
drift_ppm = offset_value * 4.340;
else
drift_ppm = offset_value * 4.069;In contrast, with the current version of this PR, one would have to write something like this: int8_t offset_value = rtc.getOffset();
float drift_ppm;
if (rtc.getOffsetMode() == "PCF8523_TwoHours")
drift_ppm = offset_value * 4.340;
else
drift_ppm = offset_value * 4.069;Although not obvious at first sight, the code above is more heavyweight than the previous one. Furthermore, although it compiles just fine, it doesn't work as expected. This kind of bug can be hard to spot (can you see it?), and would be trivially found if using the |
src/RTC_PCF8523.cpp
Outdated
|
|
||
| String RTC_PCF8523::getOffsetMode() { | ||
| String OffsetMode; | ||
| if bitRead (readOffsetReg(), 7) { |
There was a problem hiding this comment.
Just noticed this: the condition of the if should be in parentheses:
| if bitRead (readOffsetReg(), 7) { | |
| if (bitRead(readOffsetReg(), 7)) { |
|
Strangely it compiles and seems to function but yes I do get you point.
I will rectify and test it.
Thanks for your other comments.
Midnight here and I will try to absorb and work on them over the next
couple of days.
Peter
Il Dom 14 Gen 2024, 20:55 Edgar Bonet ***@***.***> ha scritto:
… ***@***.**** commented on this pull request.
------------------------------
In src/RTC_PCF8523.cpp
<#297 (comment)>:
> +/**************************************************************************/
+
+uint8_t RTC_PCF8523::readOffsetReg() {
+ uint8_t OffsetReg = read_register(PCF8523_OFFSET);
+ return OffsetReg;
+}
+
+/**************************************************************************/
+/*!
+ @brief read the offset register and return OffsetMode
+*/
+/**************************************************************************/
+
+String RTC_PCF8523::getOffsetMode() {
+ String OffsetMode;
+ if bitRead (readOffsetReg(), 7) {
Just noticed this: the condition of the if should be in parentheses:
⬇️ Suggested change
- if bitRead (readOffsetReg(), 7) {
+ if (bitRead(readOffsetReg(), 7)) {
—
Reply to this email directly, view it on GitHub
<#297 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ALANA3AEWS3MGHZZGXIO2YTYOQZ2DAVCNFSM6AAAAABBYIMWW6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTQMRQGI4TCOJWGU>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Hmm! No unfortunately I don't see the bug. I would have to try it out and then try to resolve it. :( Perhaps my intent is not quite clear. All I want to do is report back to the Serial Monitor the actual state of the register. This could be to confirm that the register has been successfully written, or alternatively to get the results of a previous unkown calibration without writing to the register in this execution of the code (rtc.calibrate commented out). The drift_ppm doesn't interest me at all in this case and I am not going to re-use the offset (-64 to +63) in any way. I merely want to report on the value previously written to the register. Without having worked on it I would see the code that I would use in a similar way to your example as follows: Serial.print("Offset mode is: ");
if (rtc.getOffsetMode() == PCF8523_TwoHours) {
Serial.println("PCF8523_TwoHours ");
} else {
Serial.println("PCF8523_OneMinute");
}When I get some time I will give it a try. |
Updated '@return'
Various minor changes and updated comments.
|
I have decided that the PR now complies reasonably well with my original intent: that was to create a method of reading the offset register following a calibration or, perhaps more importantly, obtaining and interpreting the contents of an offset register that holds a previous calibration. from enum import Enum
class Sizes(Enum):
SMALL = 1
MEDIUM = 2
LARGE = 3
print(Sizes(1).name) # 👉️ SMALL
print(Sizes(2).name) # 👉️ MEDIUM
print(Sizes(3).name) # 👉️ LARGE |
AFAIK, there isn't any. C++ is a compiled language, and the compilation process removes everything that is not strictly needed to run the program. The enumeration names are simply not uploaded to the Arduino. |
Uploaded following clang format
Upfated following possible corruption by clang-format tool
Added functions to read the PCF8523 offset register that is used for RTC calibration.
Added methods to obtain data from the offset register:
readOffsetReg();
getOffsetMode();
getOffset();
Modified files:
RTC_PCF8523.cpp
RTClib.h
Added example sketch:
pcf8523_calibrate.ino in the examples folder.
Note: The example sketch builds on the previous pcf8523.ino sketch to calibrate the PCF8523 RTC as well as controlling the contents of the offset register. Calibrating the PCF8523 would be important for most applications as they are often subject to time drift of up to one minute in a week.
Two methods of checking the contents of the offset register are provided. One is to read the raw data and then interpret it, the second obtains the settings as interpreted within the library (RTC_PCF8523.cpp)
The change only applies to the PCF8523 RTC
Test can be run with the provided example mentioned above