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

Don't try to close the dome if already closed after bad weather #74

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

XePeleato
Copy link
Contributor

According to isClosed docs:

It is called also outside of the closing sequence, to check if dome is closed when bad weather arrives.

Right now, I get a lot of "closeDomeWeather: domeCloseStart" spam in the logs when it's daytime and the weather station reports bad weather.

This commit avoids all that spam and will only log (and try to close the dome) if it's not closed.

Copy link
Member

@pkubanek pkubanek left a comment

Choose a reason for hiding this comment

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

This might be potentially dangerous. Can you instead of this put the if (isClosed() != -2) only to a log entry in ::closeDomeWeather?

According to isClosed docs: "It is called also outside of the closing sequence, to check if dome is closed when bad weather arrives."

Right now, I get a lot of "closeDomeWeather: domeCloseStart" spam in the logs when it's daytime and the weather station reports bad weather.

This commit avoids all that spam and will only log if it's not closed.
@XePeleato
Copy link
Contributor Author

This might be potentially dangerous. Can you instead of this put the if (isClosed() != -2) only to a log entry in ::closeDomeWeather?

Sure, it's done.

@XePeleato XePeleato requested a review from pkubanek April 1, 2024 15:49
Copy link
Member

@pkubanek pkubanek left a comment

Choose a reason for hiding this comment

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

I am OK with that. Please just confirm the extra message is gone. Also please don't forget to merge approved PRs.

@XePeleato
Copy link
Contributor Author

Sure, I'll double-check. About merging:
image

I don't think I can do that.
Thanks!

@XePeleato
Copy link
Contributor Author

Double-checked. This is ready for merging.

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