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

some changes are only applied when restarting node-red, not when re-deploying #12

Closed
oliverrahner opened this issue May 18, 2018 · 30 comments
Labels
bug 🐛 There is at least high chance that it is a bug!

Comments

@oliverrahner
Copy link
Collaborator

oliverrahner commented May 18, 2018

Those issues are related to KhaosT/HAP-NodeJS#580.

I don't know of a clean way to solve this, yet.

Things not working are:

  • name changes for service nodes are not always published
  • removing service nodes does not remove them from the Home app
  • state changes "into" the homekit node are not sent out or received by the Home app anymore

All of those issues are always fixed after restarting node-red, so they are "only" a pain when changing something, not during actual usage.

@oliverrahner oliverrahner added the bug 🐛 There is at least high chance that it is a bug! label May 25, 2018
@schford
Copy link

schford commented Jun 28, 2018

I too have this issue of state change into the homekit node - which is solved by restarting node red.

@ipa64
Copy link

ipa64 commented Jul 31, 2018

Hi nothing new about this annoying bug ?

@YinHangCode
Copy link

I also have the same problem.

@Flashmueller
Copy link

Same here

@oliverrahner
Copy link
Collaborator Author

As KhaosT/HAP-NodeJS#580 is now resolved, I'll start working on a possible fix when I have some spare time in the next days (or maybe weeks :-/)

@Shaquu
Copy link
Member

Shaquu commented Mar 4, 2019

For the record. I have implemented change made in KhaosT/HAP-NodeJS#580

@radokristof
Copy link
Contributor

So don’t need to restart nodered? Which version includes this?
Is this true for new Device in HomeKit as well? It will show up after re-deployment?

@Shaquu
Copy link
Member

Shaquu commented Mar 7, 2019

@radokristof I have implemented change but I think that doesn't help.
I have some ideas what's going on though.

@Shaquu Shaquu added this to the Release 0.7.0 milestone Mar 8, 2019
@Shaquu Shaquu added this to To do in Release 0.7.0 Mar 12, 2019
@Shaquu Shaquu added this to To do in Release 0.7.0 Mar 12, 2019
@oliverrahner
Copy link
Collaborator Author

@Shaquu when I mentioned KhaosT/HAP-NodeJS#580 had been implemented, I hoped that KhaosT would have done it in a way that solves the issues that he mentioned (https://github.com/KhaosT/HAP-NodeJS/pull/580#issuecomment-386829810). Unfortunately, that was not the case. The change that he made was the same that I had in place as a workaround, which didn't work :(

@Shaquu
Copy link
Member

Shaquu commented Mar 18, 2019

@oliverrahner I know I know. Unfortunately I still cannot find the problem.

@oliverrahner
Copy link
Collaborator Author

Just wanted to clarify, so you don't repeat all the mistakes I made ;)

@Shaquu
Copy link
Member

Shaquu commented Mar 18, 2019

I have an idea which make unpublish() useless.
I think I have to change this:

this.on("close", function (removed, done) {
            if (removed) {
                // This node has been deleted
                bridge.destroy();
            } else {
                // This node is being restarted
                bridge.unpublish();
                bridge = null;
                this.published = false;
            }
            done();
        });

to this:

this.on("close", function (removed, done) {
            bridge.destroy();
            bridge = null;
            self.published = false;
            done();
        });

Why? Since we "create" new Bridge every time then we should destroy connection every time.

I am pretty sure it will solve a lot...
I will try today evening.

@Shaquu
Copy link
Member

Shaquu commented Mar 18, 2019

Unpublished (unpublish()) Bridge with working server (not destroyed) could cause #22

I twisted something...

@iRonin
Copy link

iRonin commented Apr 19, 2019

+1 windows stuck in constant opening/closing state…

@Shaquu
Copy link
Member

Shaquu commented Apr 19, 2019

@iRonin thanks for feedback. I am still working on it in my free time.

@877dev
Copy link

877dev commented Apr 25, 2019

Same issue here, you only have to move the node and re-deploy and the output stops "outputting".
Works fine after re-starting node red (running on Pi3).

Thanks guys for the node and hope you find the bug! :)

@Appelg
Copy link

Appelg commented May 26, 2019

Is it possible to at least document this someplace (maybe in the INFO tab for the affected sensors?). As a new user, I just spent a couple of hours trying to figure out why my temp-sensors stopped updating :P

@radokristof
Copy link
Contributor

radokristof commented May 26, 2019 via email

@Shaquu Shaquu removed this from the Release 0.7.0 milestone Jun 6, 2019
@Appelg
Copy link

Appelg commented Jun 12, 2019

I don't know if this is obvious, but a friend of mine told me that if you choose the deploy-mode "Modified Nodes", this bug won't trigger. I just tested it and it works, so as long as you just are tweaking node settings, you don't need to restart node-red every single deploy :)

@877dev
Copy link

877dev commented Jun 12, 2019

@Appelg that’s a great tip, I had not thought of that.

I wonder if there’s any progress on a bug fix? I’d be happy to test.

@radokristof
Copy link
Contributor

@Appelg thanks however I knew that. But this will only help if you mess around with other nodes. As soon as you change something in one of your HomeKit nodes, it won't get updated (it gets updated 1 time, but not for the second time, you need to restart Node-RED again).

@Shaquu
Copy link
Member

Shaquu commented Jan 14, 2020

#184 got merged into dev branch
Please test fixes npm i node-red-contrib-homekit-bridged@1.0.0-dev.3
Remember that dev version has latest patches but can be unstable and without backward compatibility.
Current dev is not compatible with latest master release - you will have to re-add all accessories in home.app.

@radokristof
Copy link
Contributor

@Shaquu I need to re-add my accessories after update?

@Shaquu
Copy link
Member

Shaquu commented Jan 14, 2020

@radokristof yes, unfortunately dev (and next release) is not backward compatible.

Changelog here (should be complete but you know...)

@Shaquu Shaquu mentioned this issue Jan 17, 2020
Shaquu added a commit that referenced this issue Feb 23, 2020
## [1.0.0] - 2020.02.23

Lost backward compatibility. In order to make it work read this [notice](#163 (comment)).

### Fixed

-   Node id macify algorithm changed [#170](#170)
-   Corrections regarding issue [#12](#12) so that changes can be deployed without restarting node-red
-   Automatically creating a new service and replacing the old one if the service type changed
-   Automatically replacing an accessory with a new one if the accessory information changes (e.g. Name, Manufacturer, ...)
-   Video Filter value in Camera Control is now optional [#194](#194) (can be empty, before it was generated if was empty)
-   Removed updateReachability as it is deprecated (and doesn't make a difference)

### Added

-   After Service selection in node configuration Category will be automatically set to default for Service
-   Interface Name for Camera Service configuration
-   Support for new TV Remote services
-   Now first output is for onChange, second for onSet and third for camera snapshot. [#200](#200)
-   Sponsor Button on repository page

### Changed

-   Accessory Category in node configuration moved under Service selection
-   Clarify NO_RESPONSE in README
-   Update node-red version in dependencies
-   Camera Service source code to match newest improvements in homebridge-camera-ffmpeg
-   Update to latest HAP-NodeJS
-   Removed unnecessary accessory category from service node
-   Removed fields Manufacturer, Serial Number and Model from linked service nodes
-   Moved eslint and prettier configuration to package.json
-   Added automatic linting on pre-commit


Thanks to all contributors!
@radokristof @NorthernMan54 @gotofoo 

And thanks to those who also helped in testing!
@crxporter @sjorge
@Shaquu
Copy link
Member

Shaquu commented Mar 1, 2020

So how do you feel. Can it be closed now after 1.0.X release?

@crxporter
Copy link
Member

Coming up on the 2 year mark on this one!

As far as I’ve seen it’s done, I couldn’t make the redeploy bug come up on the dev branch but I haven’t actually moved to the 1 release.

@Shaquu
Copy link
Member

Shaquu commented Mar 1, 2020

@crxporter please tell me how could I make it more comfortable for you to upgrade to latest release.

@crxporter
Copy link
Member

@Shaquu I’m in the middle of replacing my Insteon hardware with zwave. My approach for a while was “don’t poke the Insteon bear” so I haven’t done much. Over the next couple of days I’ll be shutting down my old setup and building fresh with 1.0.

Might cut house power this afternoon and replace a bunch of switches...

@crxporter
Copy link
Member

Alright I did one small step. Sprinklers are 100% updated to 1.0.3 (and new node red). My other pi's will be more difficult projects.

Redeploy bug is dead, upgrade went great.

@crxporter
Copy link
Member

@Shaquu let's close it!

This is a huge one to close. Congratulations, sir and excellent work!

@Shaquu Shaquu closed this as completed Apr 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 There is at least high chance that it is a bug!
Projects
No open projects
Release 0.7.0
  
To do
Release 0.7.0
  
To do
Release v1.0.0
  
To Do
Development

No branches or pull requests