alexylem / jarvis-api

Plugin to allow controling Jarvis remotely using RestAPI
10 stars 7 forks source link

Jarvis-API does not trigger hooks ? #11

Closed Oliv4945 closed 7 years ago

Oliv4945 commented 7 years ago

Hi,

I am still playing with Jarvis thanks to Jarvis-api, and I want to use start_speaking hook in a plugin. It is triggered by the hello message, so my plugin works but the hook is not triggered when calling Jarvis-api. Did I missed something ?

Thank you

alexylem commented 7 years ago

No @Oliv4945 hooks are not triggered when Jarvis is used from the API: https://github.com/alexylem/jarvis/blob/master/utils/utils.sh#L284 This was implemented after many requests from users who did not want API calls to trigger hooks (especially Program Startup & Program Exit)

alexylem commented 7 years ago

See discussion here https://github.com/alexylem/jarvis/issues/335 Now it may be more consistent to trigger hooks from API except program related ones (startup / exit)... what do you think? Do you have any example of usage of hooks from API?

Oliv4945 commented 7 years ago

Hi @alexylem, sorry I did not found the issue #335.

From what I understand, what bothered @tigre-bleu and @renault1669 was to get program_start program_exit hooks due to Jarvis-api calling a second instance of jarvis.sh, and I think they are right. But I plan to use some hooks to a Node-Red instance thanks to an MQTT plugin, mainly for visualisation like Jarvis-face, and I want it to react in the same way if I do an order from my Respeaker, Android app or Jarvis-UI.

So I would recommend to filter program_start and program_exit hooks if $jv_api is set, but still use the hooks. Another solution is to implement a new JSON option, with 3 hooks levels like:

alexylem commented 7 years ago

Ok I will change to only filter out program_* hooks from API calls, the rest will be triggered by default. Let's see how the community reacts before adding too many options 😄

alexylem commented 7 years ago

Implémenté sur la branch beta. Pour le tester dès à présent: Menu Settings > General > Branch > beta Sinon attendre la mise à jour de ce weekend.

Oliv4945 commented 7 years ago

Nice, thank you !

Hooks work but I have weird issues with the beta branch. Api call: http://******:8080/?order=test&key=**** returns {"error": "No JSON object could be decoded"} Using Jarvis-ui the answer is {"error": "Expecting , delimiter: line 1 column 78 (char 77)"}

Log file:

mercredi 19 avril 2017, 11:27:16 (UTC+0200) ******* - - [19/Apr/2017 11:27:16] "POST / HTTP/1.1" 400 -
mercredi 19 avril 2017, 11:29:55 (UTC+0200)  ******* - - [19/Apr/2017 11:29:55] "GET /?order=test&key= *******HTTP/1.1" 400 -
alexylem commented 7 years ago

It works for me on the beta branch:

curl "http://localhost:8080?order=test&key=*****"
[{"Jarvis": "Ca fonctionne!"}]
#Wed 19 Apr 16:45:29 CEST 2017 127.0.0.1 - - [19/Apr/2017 16:45:29] "GET /?order=test&key=******* HTTP/1.1" 200 -

Please open a new ticket and make sure you haven't modified Jarvis-API for testing 😉

Oliv4945 commented 7 years ago

make sure you haven't modified Jarvis-API for testing

I hadn't, nor Jarvis-core. But I might have an mqtt plugin displaying some values I forgot to redirect :-p

Hooks are ok for me, I am closing the issue