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

[modbus] Cleaned modbus binding + extension docs #7424

Merged
merged 4 commits into from
Apr 20, 2020

Conversation

kaikreuzer
Copy link
Member

Signed-off-by: Kai Kreuzer kai@openhab.org

Signed-off-by: Kai Kreuzer <kai@openhab.org>
@kaikreuzer kaikreuzer changed the title Cleaned modbus binding + extension docs [modbus] Cleaned modbus binding + extension docs Apr 19, 2020
<feature>openhab-runtime-base</feature>
<feature>openhab-binding-modbus</feature>
<feature>openhab-transport-modbus</feature>
<bundle start-level="80">mvn:org.openhab.addons.bundles/org.openhab.binding.modbus/${project.version}</bundle>
Copy link
Member

Choose a reason for hiding this comment

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

This is different from what we do in Bluetooth and mqtt. There we have no feature.xml at all for the bundles but add the aggregated feature in the feature-build-bundle (IIRC footer.xml).

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

features/openhab-addons/src/main/resources/footer.xml

This are the features that are installed, so we should add modbus here, too. The single-features are excluded here:

<exclude name="**/org.openhab.binding.bluetooth*/**/feature.xml"/>

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I will add modbus there as well.
But what I don't understand yet: How/where are the aggregated features generated?

Copy link
Member

Choose a reason for hiding this comment

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

It slowly all comes back :-). If you tell me where to put it, I‘ll add a section on feature-definition and aggregation to the developer documentation. Since this is a bit special, I‘m not sure if it should be part of the general description for binding development.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I will add modbus there as well.

Done.

If you tell me where to put it

Hm, tricky question. We don't have anything about the general structure for an add-on yet.
https://www.openhab.org/docs/developer/buildsystem.html comes closest - maybe some info on feature definition could be added to that page, wdyt?

Copy link
Member

Choose a reason for hiding this comment

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

Added. I already wrote an explanation for the new embedding-strategy for dependencies but forgot to create a PR for that. Shame on me.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks!

@@ -1,6 +1,6 @@
<?xml version="1.0" encoding="UTF-8"?>
<features name="org.openhab.binding.modbus-${project.version}" xmlns="http://karaf.apache.org/xmlns/features/v1.4.0">
<repository>file:${basedirRoot}/bundles/org.openhab.io.transport.modbus/target/feature/feature.xml</repository>
<repository>mvn:org.openhab.core.features.karaf/org.openhab.core.features.karaf.openhab-core/${ohc.version}/xml/features</repository>
Copy link
Member

Choose a reason for hiding this comment

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

IIRC this is not working, because the transport bundle is not available (it‘s part of addons). Therefore the feature verification will fail.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do we have any documentation on what how to set repository element? This is a bit confusing to me...

Copy link
Member

Choose a reason for hiding this comment

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

Agreed. In 99+% of the bundles what you did is correct. The problem arises from the fact that the bundle depends on a feature that is not part of core but of addons. The question is, if we should integrate the transport bundle in the aggregated feature, too. Then we could add all transport bundles (including dependencies) in this feature.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, thanks, I have reverted the repo entry.

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem arises from the fact that the bundle depends on a feature that is not part of core but of addons

I was just browsing the addons repo when contemplating about openhab/openhab-core#1319 (comment) - and I came to the conclusion that the io.transport.modbus bundle simply does not belong in this repo, but should be moved to openhab-core, where all other io.transport bundles reside.
We can't move it for 2.5, but I'd strongly suggest that we move it for 3.0.

Signed-off-by: Kai Kreuzer <kai@openhab.org>
Signed-off-by: Kai Kreuzer <kai@openhab.org>
@kaikreuzer kaikreuzer requested a review from a team as a code owner April 19, 2020 19:26
@kaikreuzer
Copy link
Member Author

The Travis error does not look related to this PR...

@kaikreuzer
Copy link
Member Author

@J-N-K Can this be merged then?

@J-N-K
Copy link
Member

J-N-K commented Apr 20, 2020

The footer.xml-adjustment is missing. Without that, it will not be possible to install modbus at all.

I have left the special situation basedir.root out of the documentation, as I think it'll not be needed anymore after the transport-bundle is moved to core.

Signed-off-by: Kai Kreuzer <kai@openhab.org>
@kaikreuzer
Copy link
Member Author

Ok, does it look better now?

@J-N-K
Copy link
Member

J-N-K commented Apr 20, 2020

Just waiting for at least one CI build to succeed.

@J-N-K
Copy link
Member

J-N-K commented Apr 20, 2020

Travis fails unrelated. Let‘s merge.

@J-N-K J-N-K merged commit 64438eb into openhab:2.5.x Apr 20, 2020
@cpmeister cpmeister added this to the 2.5.5 milestone Apr 20, 2020
yfre pushed a commit to yfre/openhab-addons that referenced this pull request Apr 27, 2020
* Cleaned modbus binding + extension

Signed-off-by: Kai Kreuzer <kai@openhab.org>
Signed-off-by: Eugen Freiter <freiter@gmx.de>
LoungeFlyZ pushed a commit to LoungeFlyZ/openhab2-addons that referenced this pull request Jun 8, 2020
* Cleaned modbus binding + extension

Signed-off-by: Kai Kreuzer <kai@openhab.org>
J-N-K pushed a commit to J-N-K/openhab-addons that referenced this pull request Jul 14, 2020
* Cleaned modbus binding + extension

Signed-off-by: Kai Kreuzer <kai@openhab.org>
CSchlipp pushed a commit to CSchlipp/openhab-addons that referenced this pull request Jul 26, 2020
* Cleaned modbus binding + extension

Signed-off-by: Kai Kreuzer <kai@openhab.org>
Signed-off-by: CSchlipp <christian@schlipp.de>
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020
* Cleaned modbus binding + extension

Signed-off-by: Kai Kreuzer <kai@openhab.org>
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020
* Cleaned modbus binding + extension

Signed-off-by: Kai Kreuzer <kai@openhab.org>
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020
* Cleaned modbus binding + extension

Signed-off-by: Kai Kreuzer <kai@openhab.org>
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020
* Cleaned modbus binding + extension

Signed-off-by: Kai Kreuzer <kai@openhab.org>
DaanMeijer pushed a commit to DaanMeijer/openhab-addons that referenced this pull request Sep 1, 2020
* Cleaned modbus binding + extension

Signed-off-by: Kai Kreuzer <kai@openhab.org>
Signed-off-by: Daan Meijer <daan@studioseptember.nl>
markus7017 pushed a commit to markus7017/openhab-addons that referenced this pull request Sep 19, 2020
* Cleaned modbus binding + extension

Signed-off-by: Kai Kreuzer <kai@openhab.org>
@kaikreuzer kaikreuzer deleted the modsun branch November 13, 2021 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants