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

Unreachable Peer PLC issue since 4.0.7 #151

Closed
Tracked by #153
patrickjaigner opened this issue Feb 15, 2023 · 3 comments · Fixed by #166
Closed
Tracked by #153

Unreachable Peer PLC issue since 4.0.7 #151

patrickjaigner opened this issue Feb 15, 2023 · 3 comments · Fixed by #166
Assignees
Labels
scope:camtracker status:implemented has been implemented in some dev branch

Comments

@patrickjaigner
Copy link
Contributor

patrickjaigner commented Feb 15, 2023

First occurrence: 14.02.2023,14:47:40 on Mb02
It only happens on 3/4 of stations. Me06 is currently in maintenance and not deployed.

Mb02:
image

Mc04:
image

Md05:
image

Traceback (most recent call last):

File "C:\Users\EnclosureMc04\Documents\pyra\pyra-4.0.7\packages\core\main.py", line 116, in run
m.run(config)

File "C:\Users\EnclosureMc04\Documents\pyra\pyra-4.0.7\packages\core\modules\enclosure_control.py", line 91, in run
self.plc_interface.connect()

File "C:\Users\EnclosureMc04\Documents\pyra\pyra-4.0.7\packages\core\interfaces\plc_interface.py", line 140, in connect
self.plc.connect(self.plc_ip, 0, 1)

File "C:\Users\EnclosureMc04\AppData\Local\Programs\Python\Python310\lib\site-packages\snap7\client_init_.py", line 24, in f
check_error(code, context="client")

File "C:\Users\EnclosureMc04\AppData\Local\Programs\Python\Python310\lib\site-packages\snap7\common.py", line 89, in check_error
raise RuntimeError(error)

RuntimeError: b' TCP : Unreachable peer'

@patrickjaigner patrickjaigner added type:bug needs-triage incoming request, to be sorted labels Feb 15, 2023
@patrickjaigner patrickjaigner changed the title Unreachable Peer PLC issue since 4.0.7 on Mc04 and Md05 Unreachable Peer PLC issue since 4.0.7 Feb 15, 2023
@dostuffthatmatters dostuffthatmatters added high-priority scope:camtracker and removed needs-triage incoming request, to be sorted labels Feb 15, 2023
@dostuffthatmatters dostuffthatmatters self-assigned this Feb 15, 2023
@dostuffthatmatters
Copy link
Member

dostuffthatmatters commented Feb 15, 2023

This is probably due to 4.0.6 using python-snap7@1.1 and 4.0.7 using python-snap7@1.3. This happened because we do automatic library upgrades with backwards compatible releases (here: ^1.1).

Fix options:

  1. Fix the version at 1.1.
  2. Stay at ^1.1 and add resilience to the enclosure control procedure -> only raise an exception when the connection could not be established for X times in a row.

I prefer option 2, and we already implemented this resilience built into the PLC communication:

# Allows for PLC connection downtime of 10 minutes before an error is raised.
# Allows PLC connection to heal itself.
except Snap7Exception as e:
logger.exception(e)
now = time.time()
seconds_since_error_occured = now - self.last_plc_connection_time
if seconds_since_error_occured > 600:
raise interfaces.PLCInterface.PLCError(
"Snap7Exception persisting for 10+ minutes"
)
else:
logger.info(
f"Snap7Exception persisting for {round(seconds_since_error_occured/60, 2)}"
+ " minutes. Sending email at 10 minutes."
)

So, why do we now get a RuntimeError instead of a Snap7Exception? Because they changed it. Here is the 1.1 implementation of the error-raising function:

https://github.com/gijzelaerr/python-snap7/blob/3279ea1ca96b101021cb12890c929296e4724253/snap7/common.py#L67-L75

... and here is the 1.3 implementation:

https://github.com/gijzelaerr/python-snap7/blob/d26cfbe7da002c5527f5a1de549cc4c728640908/snap7/common.py#L75-L89

My change proposal:

 -  except Snap7Exception as e:
 +  except (Snap7Exception, RuntimeError) as e:

@dostuffthatmatters
Copy link
Member

dostuffthatmatters commented Feb 15, 2023

In my opinion, this change in the python-snap7 library is a breaking one because it is not a bug but an intentional change in behavior. Hence, it should be a 2.0 (https://semver.org/).

@patrickjaigner
Copy link
Contributor Author

Good analysis. We should discuss if backwards compatible library updates of dependencies should be done automatically for our systems.

I would strongly advice against it.
There are always risks for the instrument we can’t fully access and we don‘t have a indoor hardware setup to perform long duration tests for low occurrence errors.

dostuffthatmatters added a commit that referenced this issue Feb 16, 2023
@dostuffthatmatters dostuffthatmatters added this to the 4.0.8 - Bug Fixes milestone Feb 17, 2023
dostuffthatmatters added a commit that referenced this issue Apr 4, 2023
@dostuffthatmatters dostuffthatmatters added status:in-progress is being work on in some dev branch and removed high-priority labels May 2, 2023
dostuffthatmatters added a commit that referenced this issue May 3, 2023
@dostuffthatmatters dostuffthatmatters mentioned this issue May 3, 2023
11 tasks
dostuffthatmatters added a commit that referenced this issue May 3, 2023
@dostuffthatmatters dostuffthatmatters added status:implemented has been implemented in some dev branch and removed status:in-progress is being work on in some dev branch labels May 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope:camtracker status:implemented has been implemented in some dev branch
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants