RTC Superclass #215

Harrison-O · 2020-12-28T14:03:07Z

Wrote RTC superclass for all RTC classes. Allows for programmers to write code for any arbitrary RTC without needing C++ templates. This way, if the RTC needs to be changed to a different model, only the one line of code declaring the RTC needs to be changed. This line of code will typically be a global variable in the .ino file, meaning libraries using RTClib no longer need to use templates nor have multiple versions of each function/class for each RTC class.

Doing so required some minor changes to the existing RTC classes:

The static function members (e.g. now()) have been made non-static so that the RTC superclass' virtual function members do not produce compile errors.
There are now two begin() functions for each RTC class: one for using the RTC's current date/time and another for setting the date/time.
The RTClib.h and RTClib.cpp files have been split up into DateTime.h/cpp, TimeSpan.h/cpp and RTC.h/cpp.
All RTC classes now have a isrunning() and lostPower() function members as opposed to some only having lostPower() and others only having isrunning().

`fixDateTime()` will do nothing

Date time eqaulity

Fix date time

Update fork

Update branch

edgar-bonet

Maybe this PR would benefit from being split into smaller ones. Here are a few comments.

edgar-bonet · 2020-12-28T19:10:47Z

RTClib.h

+#if (ARDUINO >= 100)
+#include <Arduino.h> // capital A so it is error prone on case-sensitive filesystems
+// Macro to deal with the difference in I2C write functions from old and new
+// Arduino versions.
+#define _I2C_WRITE write ///< Modern I2C write
+#define _I2C_READ read   ///< Modern I2C read
+#else
+#include <WProgram.h>
+#define _I2C_WRITE send   ///< Legacy I2C write
+#define _I2C_READ receive ///< legacy I2C read
+#endif


This was originally in RTClib.cpp. Keeping it there would limit the pollution to the global namespace.

edgar-bonet · 2020-12-28T19:11:41Z

RTClib.h

-  static DateTime now();
-  static Ds1307SqwPinMode readSqwPinMode();
-  static void writeSqwPinMode(Ds1307SqwPinMode mode);
+  boolean begin(const DateTime &dt = DateTime::COMPILE_DT);


Most RTC modules can hold a small battery that enables the RTC to keep the time even when the rest of the circuit is unpowered. This is why you generally do not want to set the RTC every time the sketch starts. With this implementation of begin(), it would become impossible to not set the time at startup, which would make the backup battery useless.

Fixed by writing two begin() methods for each concrete RTC class: one for using the RTC's current date/time and another for setting the date/time.

I tend to agree with @edgar-bonet. I also see little value in second begin.

@daveythacher I wrote two being() methods as the RTC_Millis and RTC_Micros classes did not support a begin() method without a DateTime parameter whereas all the other RTC classes only had a being() method with no parameters. Writing both begin() methods ensures that other programmers don't have to worry about which RTC is being used when using begin().

edgar-bonet · 2020-12-28T19:12:35Z

RTClib.h

+  static uint32_t millisPerSecond; ///< Number of milliseconds reported by
+                                   ///< millis() per "true" (calibrated) second


This should be const.

This variable is used in the adjustDrift() and now() functions of the RTC_Millis in the same way that the microsPerSecond variable is used by the RTC_Micros class. I've now updated it so that RTC_Millis has it's own microsPerSecond variable so that the adjustDrift() function always takes a ppm parameter.

edgar-bonet · 2020-12-28T19:16:03Z

utility/RTC/RTC.h

+  /*!
+    @brief Adjust the RTC's date/time to account for RTC drift
+    @param drift The drift to adjust the date/time by
+    @note Positive values makes the clock go ahead in time and vice-versa
+  */
+  virtual void adjustDrift(const int drift);


This looks like an attempt to generalize RTC_Micros::adjustDrift(), but you seem to have misunderstood the purpose of that method. Its purpose is not to “Adjust the date/time to account for the drift”, it is instead to adjust the drift itself, or the drift rate if you wish. This adjustment does not make the time “jump” forward or back, it instead makes the clock run faster or slower.

Will fix in another pull request for another branch dedicated to implementing adjustDrift() for every concrete RTC class

edgar-bonet

Making every RTC class capable of adjustDrift() would be great, but probably not easy. Not all RTC chips have a calibration register.

edgar-bonet · 2020-12-29T13:49:53Z

keywords.txt

@@ -77,6 +78,7 @@ isEnabled32K    KEYWORD2
 #######################################
 # Constants (LITERAL1)
 #######################################
+COMPILE_DT	LITERAL1


This is not used anymore.

edgar-bonet · 2020-12-29T13:52:24Z

RTClib.cpp

@@ -347,20 +374,32 @@ void RTC_Millis::adjustDrift(int drift) { lastMillis = 1000 - drift; }
 */
 /**************************************************************************/
 DateTime RTC_Millis::now() {
-  uint32_t elapsedSeconds = (millis() - lastMillis) / millisPerSecond;
-  lastMillis += elapsedSeconds * millisPerSecond;
+  uint32_t elapsedSeconds = (millis() - lastMillis) * 1000 / microsPerSecond;


The multiplication will overflow unless now() is called at least once every 71.6 minutes (and not 49.7 days, as stated in the comment). This is the same constraint we already have with RTC_Micros. Given that this constraint is the only drawback of RTC_Micros vs. RTC_Millis, this change makes RTC_Millis redundant: it could be replaced by an alias to RTC_Micros.

Fixed by reducing RTC_Millis adjust drift precision to parts per thousand.

Parts per thousand is an extremely coarse resolution for tuning a clock. Also note that, because of the way the integer division rounds, if the user types

clock.adjustDrift(+12); // +12 ppm is about one second per day faster

his clock will speed up by +1,000 ppm, or about 86 seconds per day.

I wasn't clear enough in my wording, it's changed to a resolution of parts per thousand but the user still inputs it in parts per million. This means that

RTC_Millis rtc; rtc.adjustDrift(12);

Will have no effect. Please check my latest commit.

Even then, the programmer using adjustDrift() will be responsible for ensuring the drift adjustment is appropriate, not us; if they input some ridiculously high drift then that's their fault.

I plan on implementing ppm resolution to RTC_Millis in a separate pull request.

Your wording, and your code, were quite clear.

rtc.adjustDrift(12);

will have the effect of setting

millisPerSecond = (1000000L - 12) / 1000;

The right hand side of this expression, if evaluated as real numbers, would give 999.988. However, this is an integer division, and in C++ integer division rounds towards zero, which results in setting millisPerSecond to 999. This is a +1,000 ppm speed correction, which is both huge and very far from what the user requested.

the programmer using adjustDrift() will be responsible for ensuring the drift adjustment is appropriate, not us

Of course! But if they find, through careful calibration, that +12 ppm is the appropriate correction, they can reasonably expect that rtc.adjustDrift(12); will apply a correction that is not too far from what they requested. The library should avoid breaking such reasonable user expectations.

Alright, this is clearly becoming overly-complicated for what was supposed to be a very simple pull request. For now, I've just removed the adjustDrift() function from RTC_Millis. I've wrote adjustDrift() back into RTC_Super to enforce consistency across the RTC classes.

edgar-bonet · 2020-12-29T13:53:01Z

RTClib.cpp

-  uint32_t elapsedSeconds = (millis() - lastMillis) / millisPerSecond;
-  lastMillis += elapsedSeconds * millisPerSecond;
+  uint32_t elapsedSeconds = (millis() - lastMillis) * 1000 / microsPerSecond;
+  lastMillis += elapsedSeconds * microsPerSecond;


It should be elapsedSeconds * microsPerSecond / 1000.0. But then, extra bits would be needed in order to also store the fractional part of lastMillis, presumably in a fixed-point format.

Fixed by reducing RTC_Millis adjust drift precision to parts per thousand. May be increased without overflow error if modulo operator is used. Will investigate in future pull request for adjustDrift()

daveythacher · 2020-12-31T10:50:37Z

utility/RTC_Super.h

+    @return True if the RTC has lost power, false otherwise
+    @note Equivalent to `!rtc.isrunning()`
+  */
+  virtual boolean lostPower(void);


I would recommend this be forced to be implemented by base class. I would also consider enum where unsupported is returnable.

Wouldn't making the return type of lostPower() and isrunning() an enum cause backwards compatibility issues?

I am not sure I understand. The enum is scoped by RTC. The idea is to allow more than two return states: SUCCESS, FAILURE, UNSUPPORTED, etc. All derived classes will return on of these. These will be not driver specific.

The idea is that existing code bases using RTClib.h will expect lostPower() and isrunning() to return true or false. If this is changed to an enum of SUCCESS, FAILURE, OR UNSUPPORTED, then these code bases will no longer work.

There are a few but not so clean ways to make something like that work. Kind of crazy by some standards I am sure, so I will not even bother with it.

However I just noticed this was kind of pointless call as it is basically isRunning. Again some can be battery backed where events could be reported. This was what my first impression of this method was.

Technically this expects two different power domains to exist. Most of the time the controller and RTC share the same power tree. The battery is the secondary power source in the event of the main power failure. Technically you could use a two independent rails, but this function can be formed with isRunning.

Technically if you lost power on a software accumulated version you may struggle to detect the failure. If the RTC resets back to zero or attempts to go back to 1970 you could detect a power loss. That would indicate a power failure. However the processor event system may also have flags which report this also.

I'm not sure I understand this discussion. The RTCs I know about rely on the main power for their I2C interface. If the main power is lost, they cannot communicate, and they use the battery to keep the time (oscillator and counters running) and nothing else. When the main power comes back, if the battery was not depleted, they can report the correct time.

Those RTCs use the “lost power” flag to signal that at some point after the time was set both power sources were lost. If this flag is set the time reported by the RTC is most likely wrong.

daveythacher · 2020-12-31T10:51:27Z

utility/RTC_Super.h

+    @brief  RTC superclass for all RTC chips
+*/
+/**************************************************************************/
+class RTC_Super {


I am not a fan of this class name. It is the base class, however I do not see the need for super.

I originally called it RTC but there's already a macro of the same name. Perhaps RTC_Base?

Where is that RTC macro? I couldn't find it.

I would just change the macro. It really should be called RTC.

@edgar-bonet When I left it as RTC the automated test script detected an error with the RTC macro in
/home/runner/.arduino15/packages/arduino/tools/CMSIS-Atmel/1.2.0/CMSIS/Device/ATMEL/samd51/include/samd51j19a.h:957
@daveythacher This code is beyond my control so I can't change the macro name. You can see it in the Checks tab.

Look into the scope of macros in the preprocessor. https://stackoverflow.com/questions/6379489/scope-of-define-preprocessor-in-c

This is a good discussion: https://stackoverflow.com/questions/2321713/how-do-i-avoid-name-collision-with-macros-defined-in-windows-header-files

C++ does not get along with macros, but I still say you can work around this. It will not be clean, but should be possible. There are a few cases here worth mentioning.

Some devices have RTC modules which will not be used and conflict with the library. For these we can probably just undef the macro globally.

Some devices have RTC modules which will be used and conflict with the library. For these you would have c functions prototypes defined. Include the RTC.h, then setup the C++ method logic which calls c functions. Below that include Arduino,h and implement the c functions.

If I am not mistaken that will remove the conflict. There may be better ways. However the main thing here is the macro lives from instance to end of file. So if you included it at different point it will not exist before that point to my understanding. You can also terminate it early with undef. There is a few other approaches mentioned in that discussion.

This would also be a tad easier if those drivers were broken out to there own file.

Given that inheritance in C++ is meant to represent an “is a” relationship, what if we said that every concrete RTC “is a RealTimeClock”? It sound better to my ears than “is an RTC_Super”.

@edgar-bonet what about putting this code at the start/end of the RTC.h and RTClib.h files?

#pragma push_macro("RTC") #undef RTC ... #pragma pop_macro("RTC")

I'm stuck between the above and using RealTimeClock as the base class name.

Actually, nevermind. Using #pragma is not beginner friendly.

daveythacher · 2020-12-31T10:52:13Z

RTClib.h

@@ -323,14 +128,16 @@ enum Ds3231Alarm2Mode {
    @brief  RTC based on the DS3231 chip connected via I2C and the Wire library
 */
 /**************************************************************************/
-class RTC_DS3231 {
+class RTC_DS3231 : public RTC_Super {


Move to individual file?

Perhaps better left to another pull request? I only split the file into multiple parts because having both the parent and child classes in the same file is a bit silly.

It pains me to advocate for multiple files. I dislike long files and multiple files. However I am of the belief a class generally belongs in its own file, assuming the class is independent. I think these driver classes will be independent, however unfortunately probably short. So this could complicate code support. However it may clean things up.

I would also be interested in trying to clean up the shared logic. However if that complicated the interface or performance I guess it should stay the way it is.

daveythacher · 2020-12-31T10:52:19Z

RTClib.h

@@ -271,14 +74,16 @@ enum Ds1307SqwPinMode {
    @brief  RTC based on the DS1307 chip connected via I2C and the Wire library
 */
 /**************************************************************************/
-class RTC_DS1307 {
+class RTC_DS1307 : public RTC_Super {


Move to individual file?

daveythacher · 2020-12-31T10:53:03Z

RTClib.h

@@ -22,8 +22,10 @@
 #ifndef _RTCLIB_H_
 #define _RTCLIB_H_

+#include "utility/RTC_Super.h"


Why is base class under subfolder but not derived classes?

For compatibility with versions of the Arduino IDE before 1.5.x, only the source code files in the root and utility folders (not including source code within a folder in the utility folder) are complied. Please see the Arduino library specifications.

Fair enough, I did not know that. However I think this file should be RTC.h in root and the other in utilities.

daveythacher · 2020-12-31T10:54:53Z

utility/RTC_Super.h

+    @param dt DateTime object containing desired date/time
+    @return True if successful, false otherwise
+  */
+  virtual boolean begin(const DateTime &dt) = 0;


Not sure this is good practice. Manually calling adjust is better for interface.

This was done so that all RTC classes have both a begin(void) and begin(DateTime&) methods, as opposed to some only having the former and others the latter. Making it so that the begin() methods in the superclass are not pure virtual defeats the point of having a superclass.

Maybe begin(void) could be the only version that is mandatory. The “soft” RTCs would provide a simplistic implementation (say, setting the time to 2000-01-01T00:00:00) and provide begin(DateTime&) for backward compatibility. The soft RTCs are the only ones where not updating the RTC on startup makes little sense.

I am not sure I understand that. There is nothing that can be done if the derived extends the base class. The base class is constructed as an interface and should be abstract. The main appeal is polymorphism.

Begin is an interesting notion. Overall I see your point. However I think this could cause confusion or misusage. I see little difference between calling begin and adjust in sequence. However it is technically bad forum. However begin is technically bad form.

Personally there is an argument for dropping both begins. There are cases where it is useful like serial or async protocols which can become receivers once enabled. For binary counters or software notions I could see the argument for a begin function existing.

Or perhaps you keep both and have then do nothing. Which would prevent hammering the battery backed versions. The question is where does the seed value come from, but there is other usages for RTCs like alarm functions where this may not matter.

daveythacher · 2020-12-31T10:59:12Z

utility/DateTime.h

+    inclusive.
+*/
+/**************************************************************************/
+class DateTime {


Add daylight savings support?

That's best left for another pull-request in another branch.

This topic has been discussed multiple times here (#36, #144, #161, #204). The conclusion is that RTClib will not support that.

DST is quite a complex topic, and better left to other libraries.

I disagree. However I could see leaving it in the base class for specific RTCs who support it, but that will break the interface. DateTime is its own class which technically breaks away from the RTC interface. Alarm could be its own interface, as it is in a similar position. That one is a weird one too based on how it works, interrupts or polling.

To be clear I was technically recommending adding support. I think using enum, returning unsupported is valid. However some RTCs have built in daylight savings time modes. I was not thinking of regions per say, but rather pulling out the hardware functionality within the RTC. Granted the RTC I had in mind is not supported by this library currently.

Then that could make sense when/if support is added for such an RTC.

daveythacher · 2020-12-31T11:00:33Z

utility/DateTime.h

+    inclusive.
+*/
+/**************************************************************************/
+class DateTime {


Add support for RTCs which may not support certain functions like dates? Some way to query or report back unsupported.

Don't RTCs track the day, month, and year by definition? If not, then this is best left for another pull request. My recommendation is to make a TimeOfDay class that only stores the time and day and write a timeOfDay() method for the RTC classes. This can be enforced in the RTC base class if needed.

I want to say I have seen ones that do not support it. I just looked around a little it looks like those are grouped with RTCs but are more likely counters used for time tracking and watchdog functions. However one could use these to make a RTC.

I think the DateTime interface could be made flexible to avoid this. For the most part many RTCs do support it. It is a gray area. Some have day of the week while others do not also. However that could be viewed as out of scope for DateTime, but a nice feature perhaps.

The DS1672 is grouped with RTC and classified as RTC. However is called binary counter RTC. It also looks like some of these parts are being phased out. For the most part many do have them, and it could be fair to say that if having to build some of the DateTime they build all of it.

Here is a good list of RTCs. I did not look at it too much recently. I remember looking back at it years ago.
https://www.digikey.com/en/products/filter/clock-timing-real-time-clocks/690

DateTime is a time representation in broken-down form (year, month, day, hour, minute, second), similar to libc's struct tm. The RTC you cite as an example (DS1672) provides instead scalar time (similar to time_t). I don't see the point of adding scalar time to DateTime. The class already provides the means to convert to and from scalar time.

Another one that is a little different is DS1683, which does not work like time_t exactly. It technically can count up to 34 years worth of time, which means despite being a 32 bit counter it only has one quarter the resolution of scalar time. Now all scalar time could be converted. This is true for roughly any counter size, as in this case the hardware is just an accurate time source.

It is correct that most if not all RTCs technically do support clock and calendar functions. Some support day of the week, leap year, daylight savings, alarm functions, EEPROM, etc. Not all RTCs support the same features. Some of these features are not part of RTC's or DateTime's scope. One thought is that the driver class could implement multiple base class interfaces.

Now DateTime could also make the policy that certain features are not supported depending on implementation of driver which returns a flag accordingly. Simple but annoying.

Or on the flip side it purposefully ignores certain things and forces the application code to correct DateTime. All while requiring the DateTime to pick up in software any missing hardware component found in RTC. Therefore all of DateTime is required without exception.

DateTime will likely need to use a notion of leap year and day light at some point. In this case you are really just converting scalar time into a format. So these functions would still be used by the RTC, however the interface for them is not present to my knowledge in RTC or DateTime. DateTime is the universal abstraction of time representation so there could be an argument for it there.

Unless the plan is for the user to be completely aware that they are using one RTC versus another. In which case they cannot benefit from objects. In this case they should be kept independent and this library should not exist.

I think I have made my point, so I am going to stop here.

Maybe I'm dumb, but I honestly do not understand the point you are trying to make. The DS1683 is irrelevant to DateTime, as it is just another scalar time format. Do you know of any RTC that provide time in broken-down form as (hour, minute, second) but without (year, month, day)?

If you believe the DateTime class could benefit from a revamp, then you should open an issue, lay down your proposed additions to its interface (all new methods, their parameters, return values...), and state whether you would be willing to implement them. Otherwise we are just discussing in the vague.

Harrison-O and others added 11 commits July 30, 2020 19:11

Wrote isLeapYear() function

cb0bf8d

Wrote fixDateTime() method to fix invalid DateTime objects

0af4be5

Write additional comment specifying that if DateTime is already valid,

6f49b2b

`fixDateTime()` will do nothing

Merge pull request #1 from Harrison-O/DateTime_eqaulity

bca61c2

Date time eqaulity

Merge pull request #2 from Harrison-O/fixDateTime

ba9c0ed

Fix date time

Merge pull request #3 from adafruit/master

ae82978

Update fork

Merge pull request #4 from Harrison-O/master

dded178

Update branch

Wrote RTC superclass

7654480

Modified RTClib.h to use RTC.h

e3757d3

Fixed IDE errors

9dfa49b

Fixed compile errors

00075a4

edgar-bonet reviewed Dec 28, 2020

View reviewed changes

Moved _I2C_WRITE and _I2C_READ to RTClib.cpp

eaec406

Harrison-O marked this pull request as draft December 29, 2020 02:53

Harrison-O added 13 commits December 29, 2020 10:56

Wrote new keywords

68bed44

Fixed usage of millisPerSecond and RTC_Millis::adjustDrift()

54fcd63

Updated doxygen comments for adjustDrift()

2b98009

Wrote second begin() function for RTC classes

4a25373

Wrote adjustDraft() functions for RTC classes

c1001bc

Removed adjustDrift() for future pull request

f00f0af

Made RTC superclass public for subclasses

58e5432

Fixed compile error with begin(const DateTime&)

1614120

Removed fixDateTime() function

de9a42e

Fixed file placement in utility folder

189ddc9

Ensured that TimeSpan.h file includes Arduino.h

0975cc5

Removed COMPILE_DT constant due to compile error

3ab7669

Removed adjustDrift() from RTC for future pull request

3721491

edgar-bonet reviewed Dec 29, 2020

View reviewed changes

Harrison-O added 2 commits December 29, 2020 22:00

Fixed build errors

290bea2

Fixed adjustDrift() for RTC_Millis

41b522d

Harrison-O added 2 commits December 31, 2020 13:14

Fixed micro to milli typo

2f99da8

Fixed clang formatting issues

edc0af4

daveythacher reviewed Dec 31, 2020

View reviewed changes

Harrison-O added 13 commits December 31, 2020 21:02

Wrote adjustDrift() back in to enforce consistency

fd67d34

:w

80b5650

Fixed clang formatting issues

7213586

Removed millisPerSecond from RTC_Millis

5eba8f6

Fixed compile error involving millisPerSecond

db255bf

Renamed RTC_Super to RTC to find RTC macro

e2a981c

Changed begin(DateTime&) to non-pure virtual

8f7e656

Renamed RTC_Super to RTC_Base

c13d1c4

Changed begin(void) method for pseudo RTCs

56f3426

Updated doxygen comment

678f814

Renamed RTC_Base to RealTimeClock

2e664b4

Wrote note for RealTimeClock class

3151cf7

Changed ppm to non-const

b8d8b77

Harrison-O marked this pull request as ready for review January 4, 2021 13:56

Harrison-O marked this pull request as draft January 4, 2021 13:57

Harrison-O added 5 commits January 4, 2021 22:04

Made lostPower() virtual in base class

266cc68

Commented out adjustDrift() for future versions

84fbf61

Fixed clang formatting issue

ad85753

Fixed clang formatting issue

5cee6ba

Fixed clang formatting issue

a5a037d

Harrison-O marked this pull request as ready for review January 7, 2021 12:46

		static uint32_t millisPerSecond; ///< Number of milliseconds reported by
		///< millis() per "true" (calibrated) second

RTC Superclass #215

Are you sure you want to change the base?

RTC Superclass #215

Conversation

Harrison-O commented Dec 28, 2020 • edited Loading

edgar-bonet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edgar-bonet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Harrison-O Jan 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daveythacher Dec 31, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Harrison-O Dec 31, 2020 • edited Loading

Choose a reason for hiding this comment

daveythacher Dec 31, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Harrison-O commented Dec 28, 2020 •

edited

Loading

Harrison-O Jan 1, 2021 •

edited

Loading

daveythacher Dec 31, 2020 •

edited

Loading

Harrison-O Dec 31, 2020 •

edited

Loading

daveythacher Dec 31, 2020 •

edited

Loading