Skip to content

Conversation

drc38
Copy link
Collaborator

@drc38 drc38 commented Feb 2, 2022

Adds a counter to avoid forcing a reconnect if the connection is back online by the next 2 tries

drc38 added 2 commits February 3, 2022 08:22
Adds a counter to avoid forcing a reconnect if the connection is back online by the next 2 tries
@codecov
Copy link

codecov bot commented Feb 2, 2022

Codecov Report

Merging #355 (40c01af) into main (db8bf02) will increase coverage by 0.98%.
The diff coverage is 85.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #355      +/-   ##
==========================================
+ Coverage   85.16%   86.14%   +0.98%     
==========================================
  Files          10       10              
  Lines        1173     1184      +11     
  Branches      172      173       +1     
==========================================
+ Hits          999     1020      +21     
+ Misses        102       94       -8     
+ Partials       72       70       -2     
Impacted Files Coverage Δ
custom_components/ocpp/config_flow.py 92.00% <ø> (ø)
custom_components/ocpp/api.py 79.26% <83.33%> (+1.38%) ⬆️
custom_components/ocpp/const.py 100.00% <100.00%> (ø)
custom_components/ocpp/enums.py 100.00% <0.00%> (ø)
custom_components/ocpp/switch.py 95.31% <0.00%> (+3.12%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update db8bf02...40c01af. Read the comment docs.

@drc38 drc38 requested a review from lbbrhzn February 2, 2022 19:41
Copy link
Owner

@lbbrhzn lbbrhzn left a comment

Choose a reason for hiding this comment

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

Please make the timeout_counter configurable. This will also aid in testing.
Can you add tests for this functionality?

@drc38
Copy link
Collaborator Author

drc38 commented Feb 3, 2022

I've added the basic tests, I'm unsure how to go about replacing the websockets pong response in a fixture to cause it to raise the timeout exception though

@drc38 drc38 requested a review from lbbrhzn February 3, 2022 08:33
@lbbrhzn
Copy link
Owner

lbbrhzn commented Feb 3, 2022

I think you can disable the websocket connection by manually setting state to CLOSING.
This will prevent pongs being sent.

Copy link
Owner

@lbbrhzn lbbrhzn left a comment

Choose a reason for hiding this comment

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

WEBSOCKET_CLOSE_TRIES is confusing.
I would prefer WEBSOCKET_PING_TRIES.

@drc38 drc38 requested a review from lbbrhzn February 3, 2022 10:44
Copy link
Owner

@lbbrhzn lbbrhzn left a comment

Choose a reason for hiding this comment

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

great!

@@ -21,6 +21,7 @@
CONF_UNIT_OF_MEASUREMENT = ha.CONF_UNIT_OF_MEASUREMENT
CONF_USERNAME = ha.CONF_USERNAME
CONF_WEBSOCKET_CLOSE_TIMEOUT = "websocket_close_timeout"
CONF_WEBSOCKET_PING_TRIES = "WEBSOCKET_PING_TRIES"
Copy link
Owner

Choose a reason for hiding this comment

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

We should use a consistent case for the value.
Maybe uppercase is better? Or even better, Use strEnum, so you can easily import all conf and default?

"ws://127.0.0.1:9000/CP_3",
subprotocols=["ocpp1.6"],
) as ws:
cp = ChargePoint("CP_3_test", ws)
Copy link
Owner

Choose a reason for hiding this comment

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

Suggest to add ws.state = CLOSING after this to prevent pongs.

@lbbrhzn lbbrhzn merged commit ee534f1 into main Feb 3, 2022
@lbbrhzn lbbrhzn deleted the drc38-patch-16-2 branch February 3, 2022 10:47
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.

2 participants