-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
[Serial] Added setting to select charset for string encoding #5633
Conversation
9037568
left a comment
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.
I only see a few minor tweaks as needed.
Looks like a great fix.
| } | ||
|
|
||
| public SerialDevice(String port) { | ||
| public SerialDevice(String port, String charsetName) { |
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.
Since this is a public API, we should not destroy the original constructor. Please just add a new one.
| } | ||
|
|
||
| public SerialDevice(String port, int baud) { | ||
| public SerialDevice(String port, int baud, String charsetName) { |
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.
See comment about constructors above.
| private void setCharset(String charsetName) { | ||
| try { | ||
| if (charsetName == null) { | ||
| this.charset = Charset.defaultCharset(); |
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.
Should eliminate the extraneous use of this. throughout.
|
|
||
| logger.debug("Serial port '{}' charset '{}' set.", this.port, charsetName); | ||
| } catch (IllegalCharsetNameException e) { | ||
| logger.error("Serial port '{}' charset '{}' not found.", this.port, charsetName); |
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.
This should be a WARN, not an ERROR.
Also, I think there should probably be logic here to set the charset to the default when this exception is caught.
| * `BASE64()` enables the Base64 mode. With this mode all data received on the serial port is saved in Base64 format. All data that is sent to the serial port also has to be Base64 encoded. (This was implemented because some serial devices are using bytes that are not supported by the REST interface). | ||
| * `ON(<On string>),OFF(<Off string>)` used in conjunction with a Switch, this mapping will send specific commands to serial port and also match a serial command to specific ON/OFF state. This makes it unnecessary to use a rule to send a command to serial. | ||
| * `UP(<Up string>),DOWN(<Down string>),STOP(<Stop string>)` used in conjunction with a Rollershutter, this mapping will send specific commands to serial port. Use REGEX to parse Rollershutter postion (0-100%) coming as feedback over serial link. | ||
| * `CHARSET(<charset>)` set's the charset to be useed for converting to a String and back to bytes when writing. (e.g. UTF-8, ISO-8859-1, etc.) |
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.
One more:
"sets the charset to be used"
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.
Could you also change the first sentence in the README to eliminate the reference to ASCII ?
3df6afa to
afaaa09
Compare
- Adds a new setting to select charset for string encoding. - Problem parsing more than one setting is solved. - The BASE64 setting can now also be written as BASE64() and then it will be parsed correctly at any position inside the string. - Readme updated regarding the new charset setting and the new BASE64 settings format. Signed-off-by: David Masshardt <david@masshardt.ch>
afaaa09 to
6af5108
Compare
|
@9037568 I've just uploaded the requested changes. Please check again if everything is ok now. |
9037568
left a comment
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.
LGTM.
|
Thanks, @TheNetStriker ! |
|
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/serial-binding-sends-close-bracket-after-update-to-2-3/48801/5 |
will be parsed correctly at any position inside the string.
settings format.
Forum link: https://community.openhab.org/t/strange-problem-with-serial-binding/43230
Signed-off-by: David Masshardt david@masshardt.ch (github:
TheNetStriker)