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

Correct retry loop. #1729

Merged
merged 1 commit into from
Aug 13, 2023
Merged

Correct retry loop. #1729

merged 1 commit into from
Aug 13, 2023

Conversation

janiversen
Copy link
Collaborator

fixes #1702
fixes #1728

@janiversen janiversen merged commit bace7f3 into dev Aug 13, 2023
@janiversen janiversen deleted the solve_1702 branch August 13, 2023 07:38
Copy link
Contributor

@sefakeles sefakeles left a comment

Choose a reason for hiding this comment

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

Besides #1728, this commit causes a new issue: ModbusIOException will not be raised in case of a request timeout.

@janiversen
Copy link
Collaborator Author

it will after the retries.

@sefakeles
Copy link
Contributor

sefakeles commented Aug 13, 2023

Let's assume retries = 1 and you have a request timeout after first attempt. Then you'll have the following exception:

UnboundLocalError: cannot access local variable 'resp' where it is not associated with a value

@janiversen
Copy link
Collaborator Author

Well that is quite another error, I will look at it,

@janiversen
Copy link
Collaborator Author

solved in #1731

@sefakeles
Copy link
Contributor

Thanks, but now we have another issue: there will be retries + 1 requests before ModbusIOException is raised.

If we consider retries as the number of trials made after the timeout of initial (original) request, then this is OK. But the previous behaviour was different than that. async_execute was making retries requests at total. No more.

@janiversen
Copy link
Collaborator Author

The definition is "retries", so doing retries+1 is correct.

@sefakeles
Copy link
Contributor

OK, then this means I can have retries = 0 if I don't want the library to make any trials after the failure of initial request.

If I set retries to zero, then we still have #1728. Here's the log output:

2023-08-13 14:31:41,023 DEBUG logging:102 Connecting to 10.10.2.23:502.
2023-08-13 14:31:41,023 DEBUG logging:102 Connecting comm
2023-08-13 14:31:41,654 DEBUG logging:102 Connected to comm
2023-08-13 14:31:41,655 DEBUG logging:102 callback_connected called
2023-08-13 14:31:41,655 DEBUG logging:102 send: 0x0 0x1 0x0 0x0 0x0 0x6 0xf7 0x3 0x0 0x26 0x0 0x2
2023-08-13 14:31:41,656 DEBUG logging:102 Adding transaction 1
2023-08-13 14:31:42,595 DEBUG logging:102 recv: 0x0 0x1 0x0 0x0 0x0 0x7 0xf7 0x3 0x4 0xc1 0x23 0xb6 0x0 addr=None
2023-08-13 14:31:42,595 DEBUG logging:102 Processing: 0x0 0x1 0x0 0x0 0x0 0x7 0xf7 0x3 0x4 0xc1 0x23 0xb6 0x0
2023-08-13 14:31:42,595 DEBUG logging:102 Factory Response[ReadHoldingRegistersResponse': 3]
2023-08-13 14:31:42,595 DEBUG logging:102 Getting transaction 1
2023-08-13 14:31:42,596 DEBUG logging:102 Connection lost comm due to Server not responding
Modbus Error: [Input/Output] ERROR: No response received after 0 retries

The initial request was successful, but async_execute raised ModbusIOException.

@janiversen
Copy link
Collaborator Author

pull requests are welcome.

@sefakeles
Copy link
Contributor

There you go #1733.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Retry logic fails when retries parameter is set to 1 Pymodbus 3.4.0, bug in async_execute
2 participants