Skip to content
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

Time-to-close and further memory improvements #145

Merged
merged 22 commits into from
Apr 30, 2024

Conversation

dkerr64
Copy link
Collaborator

@dkerr64 dkerr64 commented Apr 4, 2024

fixes for issues #60 (time-to-close delay) and #143 (only reboot for settings change when necessary). Also includes cleanup and removal of unnecessary code / html pages.

Added implementation for #150

@jgstroud
Copy link
Collaborator

jgstroud commented Apr 4, 2024

at @dkerr64 can we move that last commit for the OTA to a separate PR?

@dkerr64
Copy link
Collaborator Author

dkerr64 commented Apr 4, 2024

at @dkerr64 can we move that last commit for the OTA to a separate PR?

Yes, done. PR #153

@dkerr64 dkerr64 force-pushed the time-to-close branch 7 times, most recently from 2f30944 to 8488c94 Compare April 7, 2024 02:33
@@ -859,6 +892,19 @@ void open_door() {
door_command(DoorAction::Open);
}

void TTCdelayLoop() {
if (--TTCcountdown > 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is a "service_timers_loop" in the ratgdo.cpp file. Should this go there instead?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right now in the transmitSec2 function we write the rolling code to flash every time. I think we need to add something to only write once an hour or something. This should be ok now that the device is stable enough that it stays up for days at a time. The esphome variant does something along these lines.

I think we have to do this now because closing the garage door now will trigger a lot more writes to flash.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There is a "service_timers_loop" in the ratgdo.cpp file. Should this go there instead?

I probably erred in naming this xxxLoop as this function is not part of the main loop. It executes once every 500ms using Ticker(). And it only executes during a time-to-close delay (so for a 10 second delay, it is called 20 times) to cycle the light and on completion of the timeout close the door.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right now in the transmitSec2 function we write the rolling code to flash every time. I think we need to add something to only write once an hour or something. This should be ok now that the device is stable enough that it stays up for days at a time. The esphome variant does something along these lines.

I think we have to do this now because closing the garage door now will trigger a lot more writes to flash.

If we do this (which makes sense) then we need some sort of error recovery... as we now have EspSaveCrash with a callback then maybe we have to write to the flash whenever it crashes. And is there a way to detect on boot-up that things are out-of-sync and re-initialize it... then I can remove the code file deletes from protocol change.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, so the GDO is tolerant of incrementing >1 in the rolling code, and ESPhome guys took a guess at 60. We could do the same, but note that time-to-close light flash sends two per second, so our math might be a little different... if we write once a minute, then in theory we could have >120 code increments. And we can also handle the crash case with our callback, and manually requested reboots. Which just leaves power outages as uncaught.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

btw... what is the expected life of the ESP8266 flash?

src/web.cpp Outdated Show resolved Hide resolved
src/web.cpp Show resolved Hide resolved
@dkerr64 dkerr64 force-pushed the time-to-close branch 2 times, most recently from 6f2f8f2 to f1faebe Compare April 7, 2024 19:01
@dkerr64
Copy link
Collaborator Author

dkerr64 commented Apr 7, 2024

I rebased onto latest main. Sorry for several force-push. Had several conflicts in web.cpp to get right.

I've been running with a browser open for 17 hours now, with server logs going to javascript console. All good.

I was originally thinking of having a separate URL for a separate browser to connect to, and for messages to appear in that browser. But when I looked into it, I decided that camping onto the existing server-sent-event code was easiest and most efficient on ESP memory usage, and just dumping to the browser's javascript console was much simpler than anything else. The server side code is very efficient. Just don't put any RINFO() inside the code that processes it, else infinite loop!!

I added user setting to turn this on, that setting is local to the browser (localStorage) and I hide it if you are on mobile device as makes no sense to send logs to those.

Last piece is to change HomeKit library to capture its logs.

@jgstroud
Copy link
Collaborator

jgstroud commented Apr 7, 2024

The rebase and force push is fine. I haven't had time to test this out yet. Sounds like the new memory layout option isn't showing any negative effects yet. How is your free heap looking? Can multiple clients be monitoring the serial log?

@dkerr64
Copy link
Collaborator Author

dkerr64 commented Apr 7, 2024

No negative effects. But we will have to keep an eye on things. Free heap is in good condition.

image

As for the IRAM heap. We need to keep track...

  • json string buffer... 1024 bytes.
  • log lineBuffer... 1024 bytes (could probably reduce to 512 or less, for sprintf() into for each long line.
  • log msgBuffer... 2048 bytes (holds about last 20 log lines, could increase to 4096?).
  • savedLogs... 2048 (malloc's and free'd to read saved log file into when display requested).

In other words, we need at least 2x the message buffer space as we must allocate space to read the saved file into. Now in theory I could probably read the file in chunks, but it would get rather complicated because the "head" of the circular buffer could be anywhere in the file. So long as we have the space, I prefer reading it all in at once.

@jgstroud
Copy link
Collaborator

jgstroud commented Apr 7, 2024

We can log the free IRAM heap as well, but maybe not add to web ui

https://github.com/esp8266/Arduino/blob/master/libraries/esp8266/examples/irammem/irammem.ino#L465

Copy link
Collaborator

@jgstroud jgstroud left a comment

Choose a reason for hiding this comment

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

Do you still want the Keep track of last door state change commit without the NTP client

@jgstroud
Copy link
Collaborator

I've rebased, reordered some of the commits and squashed some commits. I don't think I can update this PR, so I'm going to replace this PR with a new one

@dkerr64
Copy link
Collaborator Author

dkerr64 commented Apr 30, 2024

Do you still want the Keep track of last door state change commit without the NTP client

Yes please, I'd like to keep that. I removed all the NTP client so it just tracks it from last reboot. I find it useful and its minor stuff.

@dkerr64
Copy link
Collaborator Author

dkerr64 commented Apr 30, 2024

I've rebased, reordered some of the commits and squashed some commits. I don't think I can update this PR, so I'm going to replace this PR with a new one

Yes, definitely some squashing can be done. Thanks. I can make you a collaborator (thought you already were) or you can do a new PR if you prefer. Your choice.

@jgstroud
Copy link
Collaborator

I've rebased, reordered some of the commits and squashed some commits. I don't think I can update this PR, so I'm going to replace this PR with a new one

Yes, definitely some squashing can be done. Thanks. I can make you a collaborator (thought you already were) or you can do a new PR if you prefer. Your choice.

I was able to update the PR. Thanks. It could probably be cleaned up more, but for the most part, I'm happy with this

@jgstroud
Copy link
Collaborator

I never moved the crashlog from eeprom to filesystem. It started to seem like a big rewrite of the utility and I'm not sure we gain a whole lot from it. we can do that later though

@dkerr64
Copy link
Collaborator Author

dkerr64 commented Apr 30, 2024

I never moved the crashlog from eeprom to filesystem. It started to seem like a big rewrite of the utility and I'm not sure we gain a whole lot from it. we can do that later though

I'm fine with that. What about overwriting previous crash data, so it only attempts to keep the latest? Save that for future too?

@dkerr64
Copy link
Collaborator Author

dkerr64 commented Apr 30, 2024

I've rebased, reordered some of the commits and squashed some commits. I don't think I can update this PR, so I'm going to replace this PR with a new one

Yes, definitely some squashing can be done. Thanks. I can make you a collaborator (thought you already were) or you can do a new PR if you prefer. Your choice.

I was able to update the PR. Thanks. It could probably be cleaned up more, but for the most part, I'm happy with this

good, thanks. Embarrassingly I still have that missing semicolon commit visible to all !!! But I suppose that demonstrates integrity and that I'm not ashamed to make my trivial errors visible. :-)

@jgstroud
Copy link
Collaborator

I've rebased, reordered some of the commits and squashed some commits. I don't think I can update this PR, so I'm going to replace this PR with a new one

Yes, definitely some squashing can be done. Thanks. I can make you a collaborator (thought you already were) or you can do a new PR if you prefer. Your choice.

I was able to update the PR. Thanks. It could probably be cleaned up more, but for the most part, I'm happy with this

good, thanks. Embarrassingly I still have that missing semicolon commit visible to all !!! But I suppose that demonstrates integrity and that I'm not ashamed to make my trivial errors visible. :-)

oops, missed that one

@jgstroud jgstroud merged commit ff1ad42 into ratgdo:main Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants