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

Troubleshooting information added for UPnP (Issue #13077) #2190

Conversation

pgfeller
Copy link
Contributor

Additional technical information related to UPnP device discovery added (required for bindings that use the UPnP infrastructure of openHAB).

Signed-off-by: Patrik Gfeller [email protected]

Additional technical information related to UPnP device discovery added (required for bindings that use the UPnP infrastructure of openHAB).

Signed-off-by: Patrik Gfeller <[email protected]>
Copy link

netlify bot commented Dec 27, 2023

Thanks for your pull request to the openHAB documentation! The result can be previewed at the URL below (this comment and the preview will be updated if you add more commits).

Name Link
🔨 Latest commit abc6c85
🔍 Latest deploy log https://app.netlify.com/sites/openhab-docs-preview/deploys/65bcad7fc6e22500094c8489
😎 Deploy Preview https://deploy-preview-2190--openhab-docs-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor Author

@pgfeller pgfeller left a comment

Choose a reason for hiding this comment

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

The added information is not completely inline with the rest of the documentation as it uses docker compose as example configuration. However - I think that compose makes a lot of sense for container setups to combine other services like influxdb, grafana ... etc.).

In future it might make sense to add a documentation page on how to setup a docker stack using different services. If the reviewers do not agree I can remove the compose things. But in my opinion more information is better ...

As I use linux based servers I can not add information about the required configuration on a windows server.

@pgfeller pgfeller marked this pull request as ready for review December 27, 2023 22:16
installation/docker.md Outdated Show resolved Hide resolved
installation/docker.md Outdated Show resolved Hide resolved
installation/docker.md Outdated Show resolved Hide resolved
@pgfeller
Copy link
Contributor Author

I've added support for footnotes to the markdown parser:

https://deploy-preview-2190--openhab-docs-preview.netlify.app/docs/installation/docker.html#fn1

But the solution is not optimal yet - as it conflicts with the footer used. I'll have a look into this - as I like footnotes :-):

image

....

image

@stefan-hoehn
Copy link
Contributor

please let me know when this PR should be finally reviewed by me and eventually merged, thanks.

@pgfeller
Copy link
Contributor Author

pgfeller commented Dec 28, 2023

please let me know when this PR should be finally reviewed by me and eventually merged, thanks.

Yes - I'll do 👍 ... expected pure markdown - but the doc generator is more sophisticated than that 🙂 - so it's still WIP at the moment. But I hope to get the hang of it soon. Then I'll let you know. Thanks!

@stefan-hoehn
Copy link
Contributor

Can you explain why you did changes in .vuepress/config.js and .vuepress/styles/index.styl as this has an impact on probably many pages and what the intent is?
I am also hesitant to just add a another library "markdown-it-footnote": "^4.0.0", to the system without knowing the reason.
At least until now this wasn't necessary.

To be fair, until today I haven't looked into that topic thoroughly, so I do appreciate your effort but I need to fully understand what is happening and try it out before merging it eventually.

@pgfeller
Copy link
Contributor Author

@stefan-hoehn

Hi Stefan,

I understand your concern. The changes add support for footnotes to the markdown processor used in vuepress:

image

➡️ see the link after "subnet"

At the end of the page the generated footers are then displayed (with a link back to the paragraph):

image

I also checked the rendering on mobile devices - and it looks ok.

I like this feature to give additional information that are not absolutely necessary to solve the problem - but add more context and/or technical background to users that would like to understand more about a topic. In my opinion the main text should be short and concentrate on on how to solve the problem/create the config - but the paragraphs should concentrate on the essentials only to be readably for beginners as well. On the other hand I think advanced users might want to understand more about the background of some things and this mechanism might be a possibility to place additional info and links without blowing up the main text.

This is of course not an essential feature for the documentation, but an extension of the markdown syntax; this approach is subject to personal taste. In my opinion it adds more flexibility and add value - and might be worth consideration.

There is a risk of side effects (as with every change) - and an additional dependency. As always with software the question is valid - is the added functionality worth the added complexity ...

This is my first contribution to the documentation; I totaly understand if frequent contributors and/or maintainers are against this and I'll remove it without any bad feelings 🙂.

Let me know what you think after this explanation if you think it might be to take the (in my opinion small) risk of the change to add footnote support.

I also appreciate other opinions of docu maintaners/contrbutors of course. And in the end the change will not be merged without a review ... - it is wip (but I'm almost done).

@stefan-hoehn
Copy link
Contributor

stefan-hoehn commented Dec 29, 2023

First of all, I totally love the idea to have one more person to be involved and skilled in the documentation process <3

Let me still loop in @ghys as he is the original genie behind the whole thing (and @Confectrician to appreciate the idea).

Yannick, I think this is a nice contribution and adds value to further improve documentation. Do you have any concerns adding https://www.npmjs.com/package/markdown-it-footnote ?

The only thing that comes to my mind though that it might make sense to split the PR for that extension from the actual UPnP documentation.

@stefan-hoehn stefan-hoehn requested a review from ghys December 29, 2023 12:30
… Clarified that openHAB uses the jupnp library for UPnP support.

Signed-off-by: Patrik Gfeller <[email protected]>
Signed-off-by: Patrik Gfeller <[email protected]>
Signed-off-by: Patrik Gfeller <[email protected]>
@pgfeller
Copy link
Contributor Author

@stefan-hoehn

Hi Stefan,

from a content point of view I've finished my work on the documentation and have (re-)requested review from all whom provided feedback.

Depending on the feedback from Yannick, Jerome, you - and other senior/expirienced contributors I'll remove footnote support, or extract it into a separate PR if that is prefered. If we drop footnote support I'll just remove the additional content provided in the footnotes.

thanks and kind regards,
Patrik

@stefan-hoehn stefan-hoehn requested review from Confectrician and removed request for pgruetter January 7, 2024 10:02
@stefan-hoehn
Copy link
Contributor

@ghys Yannick can you share your thoughts here?

@ghys
Copy link
Member

ghys commented Jan 12, 2024

I'm all for it, but seeing:

image

I would still suggest placing the "caught a mistake or want to contribute" thing below the footnotes, instead of above. Footnotes are part of the content IMO.

@pgfeller
Copy link
Contributor Author

I would still suggest placing the "caught a mistake or want to contribute" thing below the footnotes, instead of above. Footnotes are part of the content IMO.

Thanks for the feedback and review. I agree with your argument - I remember that I had some problem to place the footnote above the contribution note - but I'm sure I'll figure this out eventually. I'll update the PR and let you know once it is ready for review again.

@pgfeller pgfeller force-pushed the 13077-sonos-multiple-docker-networks-interfere-with-binding branch from 358665e to cc365d8 Compare January 15, 2024 17:22
@stefan-hoehn
Copy link
Contributor

@pgfeller you (force)pushed some content. Where are we now?

@pgfeller
Copy link
Contributor Author

Hi @ghys Yannick,

until now I used the build system to create previews of the documentation. To avoid unnecessary commits I try to setup the preview build on my local machine. I noted a few things and would like to know your opinion:

  • It was not clear for me, what node.js version is used - it looks like the build system is using the latest lts; thus I use 20.11.0 locally as well. Would it make sense to add the supported engine(s) in the package.json - something like:

    "engines" : {
      "npm" : ">=10.2.4 <11",
      "node" : ">=20.11.0 <21"
    },

    I could do this if desired.

  • If I try to build the documentation with npm run build-preview-local which is probably related to encryption algros used (I also tried node.js 18 - node v18.16.1 (npm v9.5.1), some problem there ):

● Server █████████████████████████ building (10%) 1/1 modules 0 active
 

/home/pgfeller/Documents/GitHub/openhab-docs/node_modules/loader-runner/lib/LoaderRunner.js:114
                        throw e;
                        ^

Error: error:0308010C:digital envelope routines::unsupported
    at new Hash (node:internal/crypto/hash:71:19)
    at Object.createHash (node:crypto:133:10)
    at module.exports (/home/pgfeller/Documents/GitHub/openhab-docs/node_modules/webpack/lib/util/createHash.js:135:53)
    at NormalModule._initBuildHash (/home/pgfeller/Documents/GitHub/openhab-docs/node_modules/webpack/lib/NormalModule.js:417:16)
    at handleParseError (/home/pgfeller/Documents/GitHub/openhab-docs/node_modules/webpack/lib/NormalModule.js:471:10)
    at /home/pgfeller/Documents/GitHub/openhab-docs/node_modules/webpack/lib/NormalModule.js:503:5
    at /home/pgfeller/Documents/GitHub/openhab-docs/node_modules/webpack/lib/NormalModule.js:358:12
    at /home/pgfeller/Documents/GitHub/openhab-docs/node_modules/loader-runner/lib/LoaderRunner.js:373:3
    at iterateNormalLoaders (/home/pgfeller/Documents/GitHub/openhab-docs/node_modules/loader-runner/lib/LoaderRunner.js:214:10)
    at /home/pgfeller/Documents/GitHub/openhab-docs/node_modules/loader-runner/lib/LoaderRunner.js:186:6
    at context.callback (/home/pgfeller/Documents/GitHub/openhab-docs/node_modules/loader-runner/lib/LoaderRunner.js:111:13)
    at /home/pgfeller/Documents/GitHub/openhab-docs/node_modules/cache-loader/dist/index.js:196:7
    at /home/pgfeller/Documents/GitHub/openhab-docs/node_modules/neo-async/async.js:2830:7
    at done (/home/pgfeller/Documents/GitHub/openhab-docs/node_modules/neo-async/async.js:2865:11)
    at /home/pgfeller/Documents/GitHub/openhab-docs/node_modules/neo-async/async.js:2818:7
    at /home/pgfeller/Documents/GitHub/openhab-docs/node_modules/cache-loader/dist/index.js:185:9 {
  opensslErrorStack: [ 'error:03000086:digital envelope routines::initialization error' ],
  library: 'digital envelope routines',
  reason: 'unsupported',
  code: 'ERR_OSSL_EVP_UNSUPPORTED'
}

If I use define export NODE_OPTIONS=--openssl-legacy-provider this issue is gone. Is this a setup problem on my side - or shall I investigate how we can update update either the lib to support current SSL - or the project config to build locally out of the box`?

If I use export NODE_OPTIONS=--openssl-legacy-provider && npm run build-preview-local it looks better - but I still got an error:

 -> docs/installation/openhabian.md
wait Extracting site metadata...
tip Apply theme @vuepress/theme-default ...
warning Cannot read properties of undefined (reading 'toUpperCase')
tip Apply plugin container (i.e. "vuepress-plugin-container") ...
...
✔ Client
  Compiled successfully in 31.81s

✔ Server
  Compiled successfully in 26.45s

Browserslist: caniuse-lite is outdated. Please run:
  npx browserslist@latest --update-db
  Why you should do it regularly: https://github.com/browserslist/browserslist#browsers-data-updating
Language does not exist: xtend
wait Rendering static HTML...
[Vue warn]: Error in render: "TypeError: Cannot read properties of undefined (reading 'label')"

found in

---> <V05349932>
       <Content>
         <Page>
           <Layout>
             <GlobalLayout>
               <Root>
[Vue warn]: Error in render: "TypeError: Cannot read properties of undefined (reading 'label')"
...
---> <V2a460c46>
       <Content>
         <Page>
           <Layout>
             <GlobalLayout>
               <Root>
error Error rendering /addons/actions.html: false
undefined
error Error rendering /addons/bindings.html: false
undefined
error Error rendering /addons/io.html: false
undefined
error Error rendering /addons/persistence.html: false
undefined
error Error rendering /addons/transformations.html: false
undefined
error Error rendering /addons/voices.html: false
undefined
TypeError: Cannot read properties of undefined (reading 'label')
    at Proxy.render (addons/actions.md?f8f5:1:76591)
    at Vue._render (/home/pgfeller/Documents/GitHub/openhab-docs/node_modules/vue/dist/vue.runtime.common.dev.js:3559:22)
    at resolve (/home/pgfeller/Documents/GitHub/openhab-docs/node_modules/vue-server-renderer/build.dev.js:8444:27)

I'll investigate this; but if it is trivial and you can help me here it is appreciated 🙂 - is there a docu on how to build the documentation locally? I could also update the READMEof the project if desired to document what is necessary (environment and stuff).

@pgfeller
Copy link
Contributor Author

@stefan-hoehn Hi,

you (force)pushed some content. Where are we now?

I had problems to create local build (always used the build system to preview my chanes) - and unepirienced as I am tought an update to the latest code (rebase) would be a good idea to solve a potentially known problem. This introduced chaos ... I tried to restore consistency by resetting my local branch to the last commit I did prior to those rebase experiments and then force pushed to the repository to (hopefully) get a consistent state again.

At the moment I try to setup local preview build to prevent unnecessary commits to check if my changes do what I want. Once I have this working I'll include the propose change from @ghys Yannick to place the contribution links below the footnotes. But as this will involve some experiments with things I'm not too familiar (e.g. styl) I need a local environment to work efficiently.

I also asked some questions too Yannick (and a proposal for the package.json) - but my next task is to be able to build locally, then document what is required to do such a build (if there is no docu for this yet) & then finish the PR 🙂.

with kind regards,
Patrik

@stefan-hoehn
Copy link
Contributor

Maybe this helps?
The way I build the docs locally is as follows (note that at the end there are a few errors but you shouldn't worry about those):

cd .../openhab-docs (where your repo is)
export LANG=en_US.UTF-8
export LANGUAGE=en_US.UTF-8
export LC_ALL=en_US.UTF-8
npm run build-preview-local

then

cd vuepress

and start a simple http server, e.g.

python3 -m http.server

@pgfeller
Copy link
Contributor Author

There are some unexpected delays - as I'm unhappy with the fact that it requires special knowledge to build the documentation locally. Therefore I did some investigation. It seems to be possible to avoid the need for the --delete-sources parameter in the scripts. For this I experimented with an updated configuration for vuepress (migrated to typescript for intellisense support):

import { defineConfig } from "vuepress/config";
// ...
export default defineConfig({
  title: 'v4 Documentation Preview',
  description: 'This is a preview of the main parts of the documentation, found in the openhab/openhab-docs repository',
  dest: 'vuepress',
  host: 'localhost',
  patterns: [
    'docs/**/*.md',
    'addons/integrations/**/*.md', 
    '**/*.vue'],
// ...
}

Using patterns it is possible to build and serve without errors - proove of concept is working. With the use of cross-env a simple npm run serve will build and serve the documentation - assuming ruby and python 3 are present. But this I can add to docu - or check in a pre-build step.

{
  "name": "openhab-docs",
  "version": "3.0.0",
  "description": "openHAB Documentation",
  "main": "index.js",
  "scripts": {
    "build-only": "vuepress build .",
    "build-preview-local": "cross-env LANG=en_US.UTF-8 LANGUAGE=en_US.UTF-8 LC_ALL=en_US.UTF-8 NODE_OPTIONS=--openssl-legacy-provider npm run build-only",
    "build-preview": "npm run build-only",
    "postinstall": "npm dedupe && npm prune && npx browserslist --update-db",
    "prebuild-preview-local": "npm run prebuild-preview",
    "prebuild-preview": "ruby prepare-docs.rb",
    "preserve": "npm install",
    "serve": "npm run build-preview-local && cd vuepress && python3 -m http.server"
  },
  "engines": {
    "node": ">=6.13.4",
    "npm": ">=8.17.0"
  },
  "repository": {
    "type": "git",
    "url": "git+https://github.com/openhab/openhab-docs.git"
  },
  "author": "",
  "license": "EPL-2.0",
  "bugs": {
    "url": "https://github.com/openhab/openhab-docs/issues"
  },
  "homepage": "https://github.com/openhab/openhab-docs#readme",
  "dependencies": {
    "browserslist": "^4.22.2",
    "cross-env": "^7.0.3",
    "markdown-it-footnote": "^4.0.0",
    "markdownify": "^0.1.0",
    "null-loader": "^4.0.1",
    "vue-tabs-component": "^1.5.0",
    "vuepress": "^1.9.10",
    "vuepress-plugin-container": "^2.1.5",
    "vuepress-plugin-tabs": "^0.3.0",
    "webpack-serve": "^3.2.0"
  },
  "overrides": {
    "vue": "=2.6.14",
    "vue-server-renderer": "=2.6.14",
    "vue-template-compiler": "=2.6.14"
  }
}

The second thing is, that the positioning of the contribution section with css seems to be a hack - as the template has a special slot for this ... but I assume there was a reason why it was not used. So I'll investigate this as well. But will 1st work on the PR to simplify the local build, then integrate footnote support and finally add this content.

➡️ I'll open the Issues/Enhancement requests and assign them to me. Comments are of course welcome if you think this is a bad idea.

@pgfeller
Copy link
Contributor Author

➡️ I've created the two enhancement requests/issues and would like to volunteer to implement them if those changes are desired. At the moment I can not assign the issues to me - not sure how the process here works. Hint is appreciated ... but I can also search if I find the info how to assign them to me. Might also be related to the status of my account 🤔.

@stefan-hoehn
Copy link
Contributor

I'd be more than happy if you did it. Is it this PR here that you want to be assigned to? I just did that and let me know if you want me to try something out.

@pgfeller
Copy link
Contributor Author

pgfeller commented Jan 26, 2024

I'd be more than happy if you did it. Is it this PR here that you want to be assigned to? I just did that and let me know if you want me to try something out.

No - In this PR I'll simply add the content - to keep things in seperate PRs I've create two issues for [Enhancements]. If you could assign #2215 to me, that would be great. I'll create a PR with some proposed changes to simplify local work. We'll have to see if the build system will sill work with the changes as well (as some minor package updates are required).

Once that is done I plan to add footnote support (Issue #2216) - so you could assign this to me as well.

As this will delay this PR we can consider to remove the footnotes for the moment - so that the content is present in the documentation how to get UPnP related bindings to work in a docker container.

I also thinking about if I should also add a section somewhere about the benefits of compose/stack if OH is used together with e.g influxDB, grafana and other stuff (for those that not use the distro). As this made my life (backup, updates, server migration, etc ...) much easier. Also working on some widget to visualize & control the containers - but that's another story ...

image

Details page (work in progress):

image

I read about the discussion OH vs HA - I agree that OH is more complex; but the possibilities and flexibility of the system is breath taking. But the documentation can mitigate some of this - and the doc has made huge steps since I started with openHAB (I think version 1 even) ...

I think it would reduce the complexity of a full featured OH system for users with some IT background or interest. With the proper docu it is not necessary to be an expert. Caveat is, that I can only describe what is required on a linux system (as I moved away from windows on my private systems.

I wonder what would be the best place for that. Either official docu, or the docker container description - or just a forum entry. It is not a pressing thing - as I have enough to do and learn with the two issues & the other private OH projects I'm doing (there's always something to integrate and automate 🙂).

Locally it does not look bad, you only need to run npm install && npm run serve if the environment is correct (node version 16 is required now, ruby and python3) - we'll see if the required updates are a problem. But I think sooner or later updating the stack used to generate the docu is also not a bad thing.

npm 4.1.0, node 16.20.2                                                                                                                                                                          
=> npm install && npm run serve
  
  > [email protected] postinstall
  > npm dedupe && npm prune
  ...
  up to date, audited 1497 packages in 3s
  ...
  > [email protected] preserve
  > npm run build-preview-local
  > [email protected] prebuild-preview-local
  > cross-env LANG=en_US.UTF-8 LANGUAGE=en_US.UTF-8 LC_ALL=en_US.UTF-8 NODE_OPTIONS=--openssl-legacy-provider
  > [email protected] build-preview-local
  > npx vuepress build .
  
  wait Extracting site metadata...
  tip Apply theme @vuepress/theme-default ...
  warning Cannot read properties of undefined (reading 'toUpperCase')
  tip Apply plugin container (i.e. "vuepress-plugin-container") ...
  tip Apply plugin @vuepress/register-components (i.e. "@vuepress/plugin-register-components") ...
  tip Apply plugin @vuepress/search (i.e. "@vuepress/plugin-search") ...
  tip Apply plugin @vuepress/nprogress (i.e. "@vuepress/plugin-nprogress") ...
  tip Apply plugin tabs (i.e. "vuepress-plugin-tabs") ...
  
  ✔ Client
    Compiled successfully in 18.75s
  
  ✔ Server
    Compiled successfully in 13.63s
  
  wait Rendering static HTML...
  success Generated static files in vuepress.
  
  > [email protected] serve
  > cd vuepress && python3 -m http.server

Serving HTTP on 0.0.0.0 port 8000 (http://0.0.0.0:8000/) ...

Sorry - got carried away - this was a bit off topic for the PR - I hope you do not mind.

@stefan-hoehn
Copy link
Contributor

I appreciate you support on improving the docs generation a lot. I was even happy that it WORKED locally even though it runs into errors but you improving it is awesome. Generating docs locally is a huge time-server.

And yes we are putting a lot of work into docs. We are currently improving the sidebar and even integrating the docs into the UI... but now I AM getting carried away.

I think it is actually a good idea to take the footnotes out and add them later so we can finally close this PR there - thanks for proposing that Patrik.

@pgfeller
Copy link
Contributor Author

I've removed the footnote stuff & other no content related changes. I think the PR should be ready for merge. Else let me know if something needs to be changed.

Copy link
Contributor

@stefan-hoehn stefan-hoehn left a comment

Choose a reason for hiding this comment

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

Thanks, only a few small changes.

installation/docker.md Outdated Show resolved Hide resolved
installation/docker.md Outdated Show resolved Hide resolved
installation/docker.md Outdated Show resolved Hide resolved
@pgfeller
Copy link
Contributor Author

pgfeller commented Feb 2, 2024

Thanks @stefan-hoehn for the review. I'll do the changes 👍

This makes sense to have simple diff's in future updates. So I checked if there is a lint rule we could use to enforce this automatically. The closest out of the box solution I've found is MD013 - Line length. But that's not exactly what we would like to have.

A possibility would be to use Husky to check the rules on the local machine to avoid commits that violate the conventions. Josh Goldberg did write a plugin that would perform the check for single sentence per line: sentences-per-line.

As usual - this would introduce additional dependencies to the project and the commit triggers complexity ... however - it could reduce review work of the maintainers. If desired I could setup commit triggers to lint the commits locally (with, or without the rule from Josh). I would create another enhancement issue/PR for this. But I do not know what you and the other contributors/maintainers prefer: Less dependencies and complexity - or if (as I do) think that the benefits of reduced PR reviews/comments are worth it.

➡️ Let me know - and I'll follow up on this.

Fixes openhab#2215

 - resolved comments by @stefan-hoehn
 - added more information to USB section, as in my expirience to use the device id is more reliable the host system configuration changes.
@pgfeller pgfeller requested a review from stefan-hoehn February 2, 2024 08:52
Copy link
Contributor

@stefan-hoehn stefan-hoehn left a comment

Choose a reason for hiding this comment

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

So, this looks good to me now.

With regards to your proposals: Thanks for all your ideas and I am glad you come up with these great ideas but let's keep them separate in different PRs.

@stefan-hoehn stefan-hoehn merged commit 1025a02 into openhab:main Feb 5, 2024
5 checks passed
@stefan-hoehn stefan-hoehn added this to the 4.2 milestone Feb 9, 2024
@pgfeller pgfeller deleted the 13077-sonos-multiple-docker-networks-interfere-with-binding branch March 2, 2024 08:39
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.

5 participants