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

(enhancement) solves issue #1491 #1492

Closed
wants to merge 4 commits into from

Conversation

Martin-Zentner
Copy link

Immediate MQTT publish of cardid

Immediate MQTT publish of cardid
Only additional code between comments for MOD0, MODI, MODII
@pabera
Copy link
Collaborator

pabera commented Aug 26, 2021

Without looking at the code, I am assuming this is just to broadcast information into the network, but beyond that, it's not used within the Phoniebox ecosystem?!

@Martin-Zentner
Copy link
Author

Without looking at the code, I am assuming this is just to broadcast information into the network, but beyond that, it's not used within the Phoniebox ecosystem?!

From a functional side you ( @pabera ) are right.

From a requirements side the code additions require:

  • additional imports

import paho.mqtt.client as mqtt
import ssl, datetime

and

  • some configuration in the SETTINGS section of the code

@andreasbrett
Copy link
Contributor

As the creator of the MQTT daemon I dig the idea but I'd rather see this intregrated into the MQTT daemon to keep configuration centralized (why configure MQTT twice?). As a side note: if this stays in the card reader daemon, please make useFastPublish=0 by default.

@Martin-Zentner
Copy link
Author

As the creator of the MQTT daemon I dig the idea but I'd rather see this intregrated into the MQTT daemon to keep configuration centralized (why configure MQTT twice?).

I didn’t find a way to get the card reader data read immediately from the MQTT daemon.
That‘s why I simply added an additional publish to the card reader code, re-using the code from MQTT daemon.

The second configuration of MQTT in the card reader daemon is of course necessary as the same code runs in two different daemons.

When connecting to the same MQTT server the two clientid’s have to be unique, at least my mosquitto MQTT server refused the 2nd connection with the same clientid.

As a side note: if this stays in the card reader daemon, please make useFastPublish=0 by default.

Good catch. Default and comment are changed as suggested.

@andreasbrett
Copy link
Contributor

I took a different route as it was a quick change to make your idea work with the existing mqtt daemon code. Through inotify the MQTT daemon will now watch for changes to the Latest_RFID file that gets written through rfid_trigger_play.sh (that gets called through the RFID daemon). This file is written right before the card action is actually executed, so the MQTT daemon will get an immediate response and send data to the MQTT server. I chose to send all data via MQTT not just the card ID. It's the same data that gets periodically pushed but will now also be triggered by a card swipe.

Please see the changes in https://github.com/andreasbrett/RPi-Jukebox-RFID/blob/patch-4/components/smart-home-automation/MQTT-protocol/daemon_mqtt_client.py and check if it works for you. I would then create a PR. Note: you might have to install inotify via pip3 install inotify.

@andreasbrett
Copy link
Contributor

(I can't verify the code in detail right now, as I'm not near my daughter's Phoniebox)

@Martin-Zentner
Copy link
Author

I took a different route ....
[..]
Note: you might have to install inotify via pip3 install inotify.

@andreasbrett
Good news: I can confirm, that your code is working - after installing inotify as suggested. There is no noticeable difference in performance - even on my old pi model B+. ;-)

The drawback is, that my intention is slightly different from your implementation:

While you are triggering a general refresh of all publishable data after noticing that the file Latest_RFID has changed my implementation in the cardreader daemon publishes the newly read cardid just once to a specific topic, that is not periodically refreshed.

If you want to implement that behaviour in the mqtt daemon I consider these things necessary:

  1. Introduce a new topic (e.g. attribute/new_card)
  2. Exclude this topic from the general refresh
  3. Read in the new cardid from the changed file
  4. Publish the new cardid immediately (and just once) to the new topic

@andreasbrett
Copy link
Contributor

While you are triggering a general refresh of all publishable data after noticing that the file Latest_RFID has changed my implementation in the cardreader daemon publishes the newly read cardid just once to a specific topic, that is not periodically refreshed.

If you want to implement that behaviour in the mqtt daemon I consider these things necessary:

  1. Introduce a new topic (e.g. attribute/new_card)
  2. Exclude this topic from the general refresh
  3. Read in the new cardid from the changed file
  4. Publish the new cardid immediately (and just once) to the new topic

Actually I don't really understand. Publishing ALL data incl. the latest swiped card is in my opinion superior to introducing a new MQTT topic when that topic is already there (/attribute/last_card is the same as "your" /attribute/new_card). Also: why only update that topic? If a card gets swiped that will result in some other attributes to change (play state, all the current metadata). Why should we wait for the next refresh cycle to push that to your MQTT server? I'd want that info immediately not delayed.

@andreasbrett
Copy link
Contributor

publishes the newly read cardid just once to a specific topic

I think I got it now. In monitoring the last_card topic you a) don't want to check if the cardid changed and/or b) want to know if the same card got swiped again, which is not possible with the periodic attributes being published via MQTT. Especially the last part makes total sense to me, which is why I modified my implementation to make this work. I changed the topic for this event though to distinguish between periodic attribute updates and events that get only pushed when an event happens (as of now only the card swipe is an event that gets pushed).

Once a card swipe got detected it will:

  • publish the card ID to /event/card_swiped
  • publish all attributes to /attribute/...

That way the /attribute/ root will receive periodic attribute updates while the /event/ root will receive event updates only once the event happens.

@Martin-Zentner
Copy link
Author

Actually I don't really understand. Publishing ALL data incl. the latest swiped card is in my opinion superior to introducing a new MQTT topic when that topic is already there (/attribute/last_card is the same as "your" /attribute/new_card).

@andreasbrett
I think this is a simple misunderstanding.

Also: why only update that topic? If a card gets swiped that will result in some other attributes to change (play state, all the current metadata). Why should we wait for the next refresh cycle to push that to your MQTT server? I'd want that info immediately not delayed.

My intention is not to publish only the new topic - although my code in the card reader daemon does just that. ;-)

Actually I don't care, that the same card can have effects within the "phoniebox universe".
That's what the MQTT daemon takes care of already.

Additionally publishing the newly read cardid just once to a separated topic allows to trigger one-time actions outside of phoniebox in home automation systems subscribed to the new topic.

If these systems would receive the refreshed last_card value every seconds, they would execute their triggers with each refresh as they can't tell whether the published last_card value is a refresh or a new read.

For cards not registered with local phonie box actions this setup does act like a (complicated) remote RFID card reader.

@andreasbrett
Copy link
Contributor

For cards not registered with local phonie box actions this setup does act like a (complicated) remote RFID card reader.

Haven't thought of that but really dig that idea of adding a new purpose to an already running phoniebox. I think I already have some ideas to let my daughter control some of the smart home through the phoniebox that way.

BTW: If you have not yet seen my second comment: I already implemented the new topic so you could give it a new try and see if that works like you need it.

@Martin-Zentner
Copy link
Author

For cards not registered with local phonie box actions this setup does act like a (complicated) remote RFID card reader.

Haven't thought of that but really dig that idea of adding a new purpose to an already running phoniebox. I think I already have some ideas to let my daughter control some of the smart home through the phoniebox that way.

Now we are on the same page, except that I'm using this for convenience of seniors, like triggering "complicated" multimedia scenes via FHEM. I hope for a high WAF. ;-)

BTW: If you have not yet seen my second comment: I already implemented the new topic so you could give it a new try and see if that works like you need it.

I've read that with pleasure, will test and report tomorrow.

@Martin-Zentner
Copy link
Author

BTW: If you have not yet seen my second comment: I already implemented the new topic so you could give it a new try and see if that works like you need it.

I've read that with pleasure, will test and report tomorrow.

@andreasbrett

I'm glad you like my idea to add this functinoality to phoniebox.
Thanks for digging into it!

Good news is: Your latest implementation now works as desired.
I reverted to the original RFID cardreader daemon and run your latest MQTT daemon.

As expected the re-read of the file Latest_RFID adds a minor delay to the execution.
On my stone old pi that's less than 1s. If that ever matters, we can still go back to my implementation and instantly publish the event from the RFID cardreader daemon.

Now it's up to the maintainers to include this new functionality in whichever release.

@andreasbrett
Copy link
Contributor

Awesome, I will create the PR then

@Martin-Zentner
Copy link
Author

I close this pull request as @andreasbrett implemented this idea in the MQTT daemon. Thanks a lot!

I do not delete the branch, just to keep my modification to the RFID reader daemon.

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.

3 participants