-
Notifications
You must be signed in to change notification settings - Fork 56
In writeBytes return actual numBytesWritten #195
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
base: master
Are you sure you want to change the base?
In writeBytes return actual numBytesWritten #195
Conversation
93b4c0d to
4989a4b
Compare
The core change is this one:
--- src/main/java/jssc/SerialNativeInterface.java
+++ src/main/java/jssc/SerialNativeInterface.java
@@ -278,5 +278,5 @@ public class SerialNativeInterface {
* @return If the operation is successfully completed, the method returns true, otherwise false
*/
- public native boolean writeBytes(long handle, byte[] buffer) throws IOException;
+ public native int writeBytes(long handle, byte[] buffer) throws IOException;
/**
Essentially changing the return type of `writeBytes()` from `boolean` to
`int`. The reason behind this is that we can return how many bytes
actually got written, in place of just true/false.
Basically, this is a very small change. Nevertheless, there are some
consequences.
This is (kind of) a breaking change for the java API. But still not
really an issue, as it is a very easy migration. About how to migrate,
one can just do the same as done in `SerialPort.writeBytes1()` (notice
the '1' in the name). Just check if returned value matches the length of
the passed-in array. There is also a YAGNI approach (NOT RECOMMENDED!):
One could just use `writeBytes1()` instead. Which provides the old
behavior.
A more severe issue can arise because it potentially also breaks the ABI.
make sure to NOT mix usage of parts of jssc built before or after
this change! in most cases this shouldn't be an issue, as jssc unpacks
the correct version from its JAR anyway. BUT FOR THOSE WHO DID SEPARATE
THE JAVA CODE FROM THE NATIVE CODE AND INSTALL IT SEPARATELY, THEY
POTENTIALLY CAN END UP IN TROUBLE WHEN NOT TAKING CARE HERE.
In addition, this commit also contains some minor cleanups and fixes
some compiler warnings.
Related:
- java-native#129
(wip @ 17cc247)
edd78e2 to
a7ec5da
Compare
src/main/java/jssc/SerialPort.java
Outdated
| * | ||
| * @since 0.8 | ||
| */ | ||
| public boolean writeByte(byte singleByte) throws SerialPortException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be more consistent to the developers to use int everywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pro "change to int":
- Makes sense for consistency.
- IMHO API would look more clean.
- I guess a lot of code ignores the return values anyway. So the change won't really hurt.
Pro "keep boolean":
- Changing all the signatures will affect a broader surface of jssc's public API. So more consuming code will have to migrate the calls (given they check the return values in the 1st place).
- The answer is not as clear when working with
java.lang.Strings as they may produce more bytes than they contain chars.
I'm not sure which one to choose. Both sound viable. I'll have a look if it's easy to change them aswell :)
Edit: Maybe we even need to decide for every function individually? (eg keep the String/encoding affected ones as bool?)
|
Thanks! I tried to look into other |
- Add superfluous comment as requested java-native#195 (comment) - Drop legacy function `writeBytes1()` java-native#195 (comment) - Also change all the other `write..` related calls to `int` java-native#195 (comment)
- Drop legacy function `writeBytes1()` java-native#195 (comment) - Add superfluous comment as requested java-native#195 (comment) - Also change all the other `write..` related calls to `int` java-native#195 (comment) (wip @ ff8ede7)
- Drop legacy function `writeBytes1()` java-native#195 (comment) - Add superfluous comment as requested java-native#195 (comment) - Also change all the other `write..` related calls to `int` java-native#195 (comment) Edit: - Whops :) fix compiler errors. (wip @ c99ab76)
ba081e0 to
2ce0ee6
Compare
- Drop legacy function `writeBytes1()` java-native#195 (comment) - Add superfluous comment as requested java-native#195 (comment) - Also change all the other `write..` related calls to `int` java-native#195 (comment) Edit: - Add back "jssc_SerialNativeInterface.h" again (copy-paste as generated from build) java-native#195 (comment) - Fix typo. java-native#195 (comment) java-native#195 (comment) (wip @ dd6dfea)
2ce0ee6 to
64c3610
Compare
|
Before we merge, I think we should discuss this aspect of the change: As we change the signature of a native call, one will not be able to use for example a new jar with an old native binary. (no one should do that anyway) So for exmaple if some project decided to install the binary through some other means (than the auto-unpacking), like for example compiling it separately and/or installing the binary manually alongside the jar. This may cause trouble if the project will only update the jar but does not update the binary, it will NOT be compatible. As the new jar will expect numberOfBytesWritten to be returned, but the (old) native binary still will return the boolean. Do we need to take care for this case from our side? Or can we expect projects to properly update both (jar and binary) simultaneously? If we need to take care of that, I'll need to add additional backward compatibility to the native code. |
We should probably do another minor version bump if we break compat. I think this is OK. What I'm still a bit unsure about is the decision to return the written bytes in general. Since this PR doesn't link a bug report to describe why, it's hard to understand the use-case that this solves. @hiddenalpha do you mind updating the description to include the reasoning for this change so that it's more obvious? Sorry if this is self-explanatory, I just don't completely understand when writing less information than what was instructed is a non-exceptional workflow. |
This PR originates from #129 which asks about support to return the actual number of bytes written in place of just a boolean.
Here are some citations from
man 2 write:Fixes: #129