search

lincomatic / open_evse

Firmware for Open EVSE
GNU General Public License v3.0
116 stars 166 forks source link

A series of time-related cleanups #129

Closed toofishes closed 3 years ago

toofishes commented 3 years ago

I attempted to break down each logical change into individual commits with descriptive messages, so it might be easier to review this by looking at each commit individually.

There are two main methods of attack here:

All in all, this shrinks the compiled firmware size by 22 bytes of RAM and 314 bytes of Flash, or basically 1% smaller for each.

The last commit is a bit different. I wanted to make sure I didn't badly break something in serialcli, but it looks like that hasn't been tested in a while as there were several other things I needed to fix to get it to compile. Maybe it is worth dropping this code? In any case, it at least compiles now.

toofishes commented 3 years ago

@lincomatic Is there anything additional I can do to get this PR, as well as #130, reviewed? Let me know!

lincomatic commented 3 years ago

Hi, Sorry for being MIA on this. I've been very busy. I really appreciate all of the work you've put into this. The code has a lot of cobwebs in it, from including libraries that had extraneous code we didn't need, and some of the coding that looks sloppy is probably from things getting deleted over the years. I'm puzzled, though.. in my previous testing, leaving in extraneous unused code didn't increase the code size, because it seemed the linker was smart enough to leave any unused code out.

Global variables are a different story, though. I think if there are no references to the g_year/g_month, etc. we can remove them.

As for the Time.cpp/h code .. we should modify the code to use the RTC. I think the RTC library has a function to return unix time in seconds, which doesn't roll over. The tick count rollover is a real problem. It's not that a charge could run 49+ days, it's that the EVSE could easily run that long.

toofishes commented 3 years ago

Hi, Sorry for being MIA on this. I've been very busy. I really appreciate all of the work you've put into this.

Thanks! Looking back at this PR, this was pretty big, size wize. I'm happy to split this up a bit more if it would be easier to review. Just let me know, it wouldn't be a problem at all, especially if there are some changes you are definitely fine with but others you want to think more about or better understand.

The code has a lot of cobwebs in it, from including libraries that had extraneous code we didn't need, and some of the coding that looks sloppy is probably from things getting deleted over the years. I'm puzzled, though.. in my previous testing, leaving in extraneous unused code didn't increase the code size, because it seemed the linker was smart enough to leave any unused code out.

You're totally right here, sorry if I wasn't clear. I also observed that the linker is smart enough to leave unused code out. The RTC_Millis code, for example, was never actually included in the firmware image because it wasn't utilized. This removal was purely from a contributor/maintainer perspective- remove code we don't use so it doesn't confuse people.

Global variables are a different story, though. I think if there are no references to the g_year/g_month, etc. we can remove them.

👍 Agreed. I want to take a closer look at other global variables and structs in the future to see what actually needs to be global vs. what is just scratch space better done with local variables on the stack inside functions.

As for the Time.cpp/h code .. we should modify the code to use the RTC. I think the RTC library has a function to return unix time in seconds, which doesn't roll over. The tick count rollover is a real problem. It's not that a charge could run 49+ days, it's that the EVSE could easily run that long.

Sorry, I didn't call out in my PR description here a more detailed explanation that I gave as to why I think there is no need to be concerned with rollover: https://github.com/lincomatic/open_evse/pull/129/commits/65168ae21f45439a81a0a14921dddf5fa63e24f6 The summary is: As long as a charge session isn't taking more than 49.7 days, this should be a non-issue. The EVSE uptime can be days/months/years, and the math will still work fine, even with rollover, as long as the charge session itself comes to an end before a complete time wraparound.

With that said- if you're still feeling uneasy about this after reading through my commit message, I can look into the RTC/unixtime stuff to see if there is a way to retrofit this and avoid rollover altogether.

lincomatic commented 3 years ago

Again, my apologies for taking so long to respond. Yes, in the future, it would be better to break PRs into functional chunks. Wow, now that I've looked into Time.cpp .. that code really is extraneous! And I agree, the way you're using millis() is prefectly safe across a wraparound, as long as the charge session doesn't run 49+ days. the SERIALCLI code is deprecated, and hasn't been maintained in years. I had actually planned to completely remove it, but never got around to it, but thanks for your fixes to that.

Thanks very much for all of your hard work.