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

S-Bus transport protocol. Derived from Modbus protocol #4025

Open
wants to merge 48 commits into
base: main
Choose a base branch
from

Conversation

cipianpascu
Copy link

@cipianpascu cipianpascu commented Jan 7, 2024

This is the first attempt to implement the S-Bus G4 transport protocol.
The code is derived from the Modbus transport protocol and has the following two additions:

  1. TCP protocol is replaced with UDP protocol
  2. All the protocol queries have a subnetId byte in addition to the deviceId and functionId.
    Please have a look to what I did, and let me know about the additional changes/fixes required for this new transport protocol/binding.

@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/new-transport-protocol-s-bus-g4/152769/1

@wborn wborn requested a review from ssalonen January 7, 2024 10:47
Ciprian Pascu and others added 26 commits January 17, 2024 20:57
Signed-off-by: Ciprian Pascu <contact@ciprianpascu.ro>
Signed-off-by: Ciprian Pascu <contact@ciprianpascu.ro>
Signed-off-by: Ciprian Pascu <contact@ciprianpascu.ro>
Signed-off-by: Ciprian Pascu <contact@ciprianpascu.ro>
Signed-off-by: Ciprian Pascu <contact@ciprianpascu.ro>
Signed-off-by: Ciprian Pascu <contact@ciprianpascu.ro>
Signed-off-by: Kai Kreuzer <kai@openhab.org>
Signed-off-by: Ciprian Pascu <contact@ciprianpascu.ro>
Signed-off-by: Ciprian Pascu <contact@ciprianpascu.ro>
…3957)

Also-by: Connor Petty <mistercpp2000+gitsignoff@gmail.com>
Signed-off-by: J-N-K <github@klug.nrw>
Signed-off-by: Ciprian Pascu <contact@ciprianpascu.ro>
`isEmpty()` expresses the intent more clearly and is therefore preferred.
Counting the number of elements can also be an expensive operation e.g. when using linked lists.

Signed-off-by: Wouter Born <github@maindrain.net>
Signed-off-by: Ciprian Pascu <contact@ciprianpascu.ro>
…3997)

These array creations are unnecessary because arrays are created automatically for methods using varargs.

Signed-off-by: Wouter Born <github@maindrain.net>
Signed-off-by: Ciprian Pascu <contact@ciprianpascu.ro>
Removes redundant modifiers from the code.
These modifiers redeclare the default modifiers that apply to interfaces, enums etc.

Signed-off-by: Wouter Born <github@maindrain.net>
Signed-off-by: Ciprian Pascu <contact@ciprianpascu.ro>
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
Signed-off-by: Ciprian Pascu <contact@ciprianpascu.ro>
Java assertions are disabled by default so in this PR they are replaced/removed where applicable.

Signed-off-by: Wouter Born <github@maindrain.net>
Signed-off-by: Ciprian Pascu <contact@ciprianpascu.ro>
The first parameter should be the expected value and the second parameter the actual value.

Signed-off-by: Wouter Born <github@maindrain.net>
Signed-off-by: Ciprian Pascu <contact@ciprianpascu.ro>
Often the type can be inferred so the diamond operator can be used to simplify the code.

Signed-off-by: Wouter Born <github@maindrain.net>
Signed-off-by: Ciprian Pascu <contact@ciprianpascu.ro>
An infinite recursion would occur when calling this method.
I did not find any add-ons using this method but that could be due to this bug.

Signed-off-by: Wouter Born <github@maindrain.net>
Signed-off-by: Ciprian Pascu <contact@ciprianpascu.ro>
* Simplify assertions

Using the appropriate assertion methods results in less and easier to read code as well as better error messages when assertions fail.

Signed-off-by: Wouter Born <github@maindrain.net>
Signed-off-by: Ciprian Pascu <contact@ciprianpascu.ro>
A static inner class does not keep an implicit reference to its enclosing instance.
This prevents a common cause of memory leaks and uses less memory per instance of the class.

Signed-off-by: Wouter Born <github@maindrain.net>
Signed-off-by: Ciprian Pascu <contact@ciprianpascu.ro>
These argument mismatches cause wrong messages being logged and thrown in exceptions.

Signed-off-by: Wouter Born <github@maindrain.net>
Signed-off-by: Ciprian Pascu <contact@ciprianpascu.ro>
This helps with stopping the proliferation of unnecessary semicolons.

Signed-off-by: Wouter Born <github@maindrain.net>
Signed-off-by: Ciprian Pascu <contact@ciprianpascu.ro>
* Iterate using Map entries

Iteration using Map entries is preferred because it is more efficient and helps preventing NPEs.

Signed-off-by: Wouter Born <github@maindrain.net>
Signed-off-by: Ciprian Pascu <contact@ciprianpascu.ro>
* Simplify adding elements to Collections

This optimizes and simplifies the code that adds elements to Collections.

Signed-off-by: Wouter Born <github@maindrain.net>
Signed-off-by: Ciprian Pascu <contact@ciprianpascu.ro>
…4010)

Abstract classes should not have public constructors.
Constructors of abstract classes can only be called in constructors of their subclasses.
So there is no point in making them public.
The protected modifier should be enough.

Signed-off-by: Wouter Born <github@maindrain.net>
Signed-off-by: Ciprian Pascu <contact@ciprianpascu.ro>
Signed-off-by: Wouter Born <github@maindrain.net>
Signed-off-by: Ciprian Pascu <contact@ciprianpascu.ro>
Karaf does not automatically install the feature after `dependency="true"` was added in openhab#3814.

Signed-off-by: Wouter Born <github@maindrain.net>
Signed-off-by: Ciprian Pascu <contact@ciprianpascu.ro>
wborn and others added 17 commits January 17, 2024 20:57
Fixes various issues including wrong parameter names, references, links and dangling JavaDocs.

Signed-off-by: Wouter Born <github@maindrain.net>
Signed-off-by: Ciprian Pascu <contact@ciprianpascu.ro>
Signed-off-by: Holger Friedrich <mail@holger-friedrich.de>
Signed-off-by: Ciprian Pascu <contact@ciprianpascu.ro>
For the changelog, see:

https://github.com/jmdns/jmdns/milestone/11?closed=1

Signed-off-by: Kai Kreuzer <kai@openhab.org>
Signed-off-by: Ciprian Pascu <contact@ciprianpascu.ro>
Caused by openhab#3957

Signed-off-by: Wouter Born <github@maindrain.net>
Signed-off-by: Ciprian Pascu <contact@ciprianpascu.ro>
Signed-off-by: Jan N. Klug <github@klug.nrw>
Signed-off-by: Ciprian Pascu <contact@ciprianpascu.ro>
…b#3960)

* [Audio] Add piped audio stream and fix pcm format usage

Signed-off-by: Miguel Álvarez <miguelwork92@gmail.com>
Signed-off-by: Ciprian Pascu <contact@ciprianpascu.ro>
Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
Signed-off-by: Ciprian Pascu <contact@ciprianpascu.ro>
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Ciprian Pascu <contact@ciprianpascu.ro>
…penhab#4029)

* Fix Instant serialization/deserialization regression

Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>

* Consolidate serialization and deserialization in same type adapter

Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>

* Simplify deserializer

Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>

---------

Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
Signed-off-by: Ciprian Pascu <contact@ciprianpascu.ro>
Signed-off-by: Wouter Born <github@maindrain.net>
Signed-off-by: Ciprian Pascu <contact@ciprianpascu.ro>
When the CPU load of a system is high these timeouts may not be realistic.

Fixes openhab#3254

Signed-off-by: Wouter Born <github@maindrain.net>
Signed-off-by: Ciprian Pascu <contact@ciprianpascu.ro>
Upgrades Jollyday 0.5.10 (de.jollyday) to 0.23.2 of a more actively maintained fork (de.focus-shift).

* This adds many missing holidays.
* Also removes the workaround for the Danish Great Prayer Day introduced by openhab#3573.

For release notes, see:

https://github.com/focus-shift/jollyday/releases

Fixes openhab#3544

Signed-off-by: Wouter Born <github@maindrain.net>
Signed-off-by: Ciprian Pascu <contact@ciprianpascu.ro>
Fix openhab#4037

Signed-off-by: Jan N. Klug <github@klug.nrw>
Signed-off-by: Ciprian Pascu <contact@ciprianpascu.ro>
It has been reported several times that add-ons were not properly installed / missing after an upgrade or the installation of incompatible add-ons resulted in broken installations.

After an upgrade (or clean cache) the `AddonHandler`s try to re-install the add-ons from the download cache (`<userdata>/marketplace`). This happens without checking compatibility. This was needed before OH4, because the cache was the only source providing information about installed add-ons. This is now different, since we store the add-on information in a JSON database, so the UIDs of the add-ons are known.

This PR changes improves the add-on services. It now

1. Reads the information about the installed add-ons from the database and sets the installation status based on information from the handlers.
2. Removes all add-ons that are not installed from the JSON database and remembers their UIDs.
3. Refreshes the remote add-on list (including check for compatibility if not disabled).
4. Tries installation of the add-ons remembered in step 2. Since incompatible add-ons are missing in the add-on list, their installation fails and a warning is logged.

This PR is has two corresponding PR in openhab-distro and openhab-linuxpkg to ensure that the upgrade script and `openhab-cli` also clear the marketplace cache.

Signed-off-by: Jan N. Klug <github@klug.nrw>
Signed-off-by: Ciprian Pascu <contact@ciprianpascu.ro>
The class of the the broken provider should be logged, not the class of the command description.

Signed-off-by: Jan N. Klug <github@klug.nrw>
Signed-off-by: Ciprian Pascu <contact@ciprianpascu.ro>
…ab#4043)

Signed-off-by: Jan N. Klug <github@klug.nrw>
Signed-off-by: Ciprian Pascu <contact@ciprianpascu.ro>
Signed-off-by: Jan N. Klug <github@klug.nrw>
Signed-off-by: Ciprian Pascu <contact@ciprianpascu.ro>
Ciprian Pascu added 4 commits January 20, 2024 11:32
Signed-off-by: Ciprian Pascu <contact@ciprianpascu.ro>
Signed-off-by: Ciprian Pascu <contact@ciprianpascu.ro>
Signed-off-by: Ciprian Pascu <contact@ciprianpascu.ro>
@ssalonen
Copy link
Contributor

I find this hard to review, the modifications to modbus are not in a separate commit. Can you point to the place where the protocol differs?

would it be possible to extend the modbus transport and binding to support also this case?

…bus calls

Signed-off-by: Ciprian Pascu <contact@ciprianpascu.ro>
@cipianpascu
Copy link
Author

cipianpascu commented Jan 27, 2024

Hey Sami ( @ssalonen ),

True, due to package renaming, it is not that easy to indentify the core changes. But I will try to highlight them.
First, I introduced the subnetId in j2sbus (forked from jamod) library:
https://github.com/cipianpascu/j2sbus/blob/feature/openhab/src/main/java/ro/ciprianpascu/sbus/msg/ModbusMessage.java#L75
https://github.com/cipianpascu/j2sbus/blob/feature/openhab/src/main/java/ro/ciprianpascu/sbus/ModbusCoupler.java#L145
paying attention to the length of the serialized message:
https://github.com/cipianpascu/j2sbus/blob/feature/openhab/src/main/java/ro/ciprianpascu/sbus/io/ModbusUDPTransport.java#L80

After that, I introduced the subnetId in the io.transport.sbus module (ModbusReadRequestBlueprint, ModbusWriteRegisterRequestBlueprint, ModbusWriteCoilRequestBlueprint) so that I can transfer the subnetId from the thing to the sbus requests in the ModbusLibraryWrapper:
https://github.com/openhab/openhab-core/pull/4025/files#diff-3a65c1afa0e4d747416822744bf25092732b40984a277583834f53a06b7d9545
https://github.com/openhab/openhab-core/pull/4025/files#diff-d88be59adf03f3211a19cf34f2cc0598e9d28b5ad974e919a44a1475e9296875
https://github.com/openhab/openhab-core/pull/4025/files#diff-c8270491534da242e8da50a1adc400087d11fc95fa36d0e41b5dc8896629f724

https://github.com/openhab/openhab-core/pull/4025/files#diff-ee24094074fe145ec56143be6e0806f7317702c193cc971cd502594071ed3445

Given the above changes, I don't see a way at this moment to appply the sbus changes to jamod library without breaking the existing functionality (and modbus specs). But it might be a posibility to add j2sbus library as a dependency to io.transport.modbus bundle and then ammend the ModbusLibraryWrapper.createRequest methods with a new if branch around the subnetId and calling. But this time, I find it a bit problematic to deal with the ro.ciprianpascu.sbus.msg.ModbusRequest interface.

Also, I created a unifiedDiff.zip, which ignores all the package and comments changes. This way, your review process should become easier.
Given the above, if you still believe that merging my changes into the modbus implementation is a good option, then I could give it a try.

Thank you,
Ciprian

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.