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

Add support of PlcDINT for BigInteger in PlcValueHandler #962

Merged
merged 3 commits into from Jun 5, 2023

Conversation

PatrykGala
Copy link
Contributor

@PatrykGala PatrykGala commented May 27, 2023

  1. The uncaught PlcUnsupportedDataTypeException from PlcValueHandler caused the CompletableFuture not to complete, the OpcuaPlcDriverTest hangs on CompletableFuture.get()
  2. The root of the problem is the missing support of BigInteger in PlcValueHandler.java

@PatrykGala PatrykGala changed the title Add support of PlcULINT for BigInteger in PlcValueHandler Add support of PlcDINT for BigInteger in PlcValueHandler May 27, 2023
Copy link
Contributor

@chrisdutz chrisdutz left a comment

Choose a reason for hiding this comment

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

Please don't disable tests ... the test isn't blocking on my computer or on the CI servers, so if your changes make it block, we probably should take care of this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't just disable tests without opening a new issue for it ... this way we have already disabled very many tests, tests we don't know about or forget.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am enabling in this PR, the test was disabled in 0af938f

@@ -83,6 +83,9 @@ public static PlcValue of(Object[] values) {
if (value instanceof Long) {
return PlcLINT.of(value);
}
if (value instanceof BigInteger) {
return PlcDINT.of(value);
Copy link
Contributor

Choose a reason for hiding this comment

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

Woudln't a LINT be the better match, as a BigInteger could actually be even bigger than the DINT value space?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, PlcLINT is better choose. I've pushed the change.

@@ -195,7 +195,7 @@ public CompletableFuture<PlcReadResponse> read(PlcReadRequest readRequest) {
future.complete(new DefaultPlcReadResponse(request, status));
return;
}
} catch (ParseException e) {
} catch (Exception e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What was the reasoning behind this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The uncaught PlcUnsupportedDataTypeException thrown by PlcValueHandler prevents the CompletableFuture from completing, causing the thread to hang on CompletableFuture.get(). By making this change, the test will now complete with an Exception instead of hanging indefinitely.

@splatch
Copy link
Contributor

splatch commented Jun 3, 2023

@chrisdutz if you're ok with changes (PlcLINT is in place) I'd proceed with merging.

@chrisdutz
Copy link
Contributor

Please enable the test first

@splatch
Copy link
Contributor

splatch commented Jun 4, 2023

Please enable the test first

As far I can see this PR enables test which was disabled.

@chrisdutz
Copy link
Contributor

Indeed it does... So ignore my mumbling 😁

@sruehl sruehl merged commit bd13295 into apache:develop Jun 5, 2023
9 of 12 checks passed
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

4 participants