Clon1998 / mobileraker_companion

Companion for mobileraker, enabling push notification.
MIT License
209 stars 12 forks source link

change to custom notifications #49

Closed TomW1605 closed 1 year ago

TomW1605 commented 1 year ago

i would like to propose changing custom notifications to use a klipper style extended gcode command, eg MOBILE_MESSAGE, instead of overloading the native M117. that way you could write macros that send notifications to the app but dont display them on the printer screen. also the $MR$: format of the message causes issues if you are already overloading the M117 command and want to send the args through (it trys to process the $ as part of the command)

Clon1998 commented 1 year ago

If you show me a way that allows me/the companion to receive messages in a push-based way just like it is possible with M117 I will happily include it.

TomW1605 commented 1 year ago

ill have a look. i assumed there was some klipper hook that sent every gcode message to the companion to parse and you could just add the custom one to the list you pars but if klipper is doing something special with M117 this may be a lot harder than i thought

TomW1605 commented 1 year ago

so i had a look at the code and can see you are not processing gcode. that makes it harder but i have found a way to make this work. i will admit its not great so i want to run it by you before i do any more work on it

basically you need to create a gcode macro with a variable that is set within the macro. eg:

[gcode_macro MOBILE_NOTIFY]
variable_message: ""
gcode:
  RESPOND TYPE=command MSG="MOBILE_NOTIFY: { params.MESSAGE }"
  SET_GCODE_VARIABLE MACRO=MOBILE_NOTIFY VARIABLE=message VALUE='"{ params.MESSAGE }"'

then you add "gcode_macro MOBILE_NOTIFY": None to the printer object subscriptions on line 158 of mobileraker.py

then when the macro is run with the MESSAGE paramater (eg, MOBILE_NOTIFY MESSAGE=11111) you will get a notify_status_update message with the data

{
  "jsonrpc": "2.0",
  "method": "notify_status_update",
  "params": [
    {
      "gcode_macro MOBILE_NOTIFY": {
        "message": "11111"
      }
    },
    680481.006052344
  ]
}

this would then need to be processed but i think that should be fairly easy.

i understand that needing a macro in printer config is not elegant but unfortunately it was the only way i could find to manually trigger the notify_status_update event (or any other event) other than M117 or other things that already do something. let me know if you think this is something worth continuing with and is so, how you want to proceed. i am happy to take a stab at the processing of the custom message and sending the notification if you want

Clon1998 commented 1 year ago

so i had a look at the code and can see you are not processing gcode. that makes it harder but i have found a way to make this work. i will admit its not great so i want to run it by you before i do any more work on it

basically you need to create a gcode macro with a variable that is set within the macro. eg:

[gcode_macro MOBILE_NOTIFY]
variable_message: ""
gcode:
  RESPOND TYPE=command MSG="MOBILE_NOTIFY: { params.MESSAGE }"
  SET_GCODE_VARIABLE MACRO=MOBILE_NOTIFY VARIABLE=message VALUE='"{ params.MESSAGE }"'

then you add "gcode_macro MOBILE_NOTIFY": None to the printer object subscriptions on line 158 of mobileraker.py

then when the macro is run with the MESSAGE paramater (eg, MOBILE_NOTIFY MESSAGE=11111) you will get a notify_status_update message with the data

{
  "jsonrpc": "2.0",
  "method": "notify_status_update",
  "params": [
    {
      "gcode_macro MOBILE_NOTIFY": {
        "message": "11111"
      }
    },
    680481.006052344
  ]
}

this would then need to be processed but i think that should be fairly easy.

i understand that needing a macro in printer config is not elegant but unfortunately it was the only way i could find to manually trigger the notify_status_update event (or any other event) other than M117 or other things that already do something. let me know if you think this is something worth continuing with and is so, how you want to proceed. i am happy to take a stab at the processing of the custom message and sending the notification if you want

🚀 that is an elegant way to listen to the GCode macros and one I was not aware of. However, moving forward I think it would be best to keep both options for now. The m117 one is way easier to use for new users who don't want to configure additional GCode macros. I don't mind if you want to contribute and add this feature, but please use the rewrite branch #45. I put a lot of effort into rewriting and restructuring the code to make it easier to extend and split up the logic responsible for creating notifications and keeping track of the latest Klipper/moonraker data. Also, I am not a python-dev so feel free to review or rewrite other sections if you fancy it.

Clon1998 commented 1 year ago

The easiest would be to just add a GCode Dto/Object/Variable to the data_sync_service. Include the specific macro in the query + subscription jrpc request and than just add some code to the parsing _parse_objects and modify take_snapshot. The notification logic will take care of the rest.

TomW1605 commented 1 year ago

cool ill give it a shot, and i agree that keeping both methods makes sense since you still need the M117 processing for displaying it in the app.

do you think the macro name and veriable should be configurable or do you think having that set is ok?

also sorry i forgot about the rewrite branch, i had updated to it but then when i compared my local directory to the github i forgot to switch the github branch and reinstalled the current release branch, ill port my stuff over to the rewrite branch and add the stuff to the sync and snapshot stuff

Clon1998 commented 1 year ago

I would say we only allow a specific name for both the macro and the variable we store. You can pick any name to be honest, but I would like to also prefix the macro with MR just to keep it consistent. Also, will the templating/placeholders still work?

TomW1605 commented 1 year ago

ok how about MR_NOTIFY and MR_MESSAGE?

as for placeholders i think that should still work since after the initial message from moonraker it will just inject it into the existing pipeline used by the custom M117 messages

Clon1998 commented 1 year ago

ok how about MR_NOTIFY and MR_MESSAGE?

as for placeholders i think that should still work since after the initial message from moonraker it will just inject it into the existing pipeline used by the custom M117 messages

You can keep the parameter/variable name of the macro as message. Let's just rename the macro to MR_NOTIFY

TomW1605 commented 1 year ago

so i have gotten this working in a usable way. i have updated the macro to also accept a title and then convert the title and message into the same format used by the custom M117 message. that way when they get to the mobileraker code they can be handled exactly the same as M117. and since the app is connecting directly to moonraker for its displayed M117 messages this does not impact that and only triggers a notification

[gcode_macro MR_NOTIFY]
variable_message: ""
gcode:
  {% if 'MESSAGE' in params|upper %}
    {% if 'TITLE' in params|upper %}
      {% set msg = "$MR$:"+params.TITLE+"|"+params.MESSAGE %}
      RESPOND TYPE=command MSG="MR_NOTIFY: { params.TITLE } - { params.MESSAGE }"
    {%else %}
      {% set msg = "$MR$:"+params.MESSAGE  %}
      RESPOND TYPE=command MSG="MR_NOTIFY: { params.MESSAGE }"
    {% endif %}

    SET_GCODE_VARIABLE MACRO=MR_NOTIFY VARIABLE=message VALUE='"{ msg }"'
  {%else %}
    RESPOND TYPE=error MSG="Must provide MESSAGE parameter"
  {% endif %}

image

i am not a fan of needing a more complex macro but it turned out to be necessary for supporting sending the title anyway. i tried storing the title and message separately however unfortunately if you only change one only that one is sent in the notification and i decided having a slightly more complex macro was better than having to store and compare that extra data on the python side. this way if either change, both are sent but if you don't change either of them and rerun the macro nothing is sent just as with the M117.

let me know what you think and if you are ok with this method i will open a PR. if you would prefer the message is injected into the M117 pipeline later that's fine, but i will have to take a stab at that tomorrow (currently 11:55pm)

(EDIT: small change to the macro to make the echo response look better)

Clon1998 commented 1 year ago

Nice work! However, I think I found an even better way that also does not rely on the GCode variables. Instead of using the macro itself, we could make use of the GCodeResponse-Notification. As far as I can tell, the notifications are created for all action_response_info calls. By doing that, the user could modify the macro name if he wants to (I'd keep one macro as an example in the readMe) and we do not need to save anything in the GCode variables. Also, I asked ChatGPT to improve the Jinja2 template format for this macro :).

[gcode_macro MR_NOTIFY]
description: Allows you to send a custom notification via Mobileraker without using the M117 command
gcode:
    {% set msg = "$MR$:" ~ (params.TITLE ~ "|" if 'TITLE' in params|upper else "") ~ params.MESSAGE %}

    {% if 'MESSAGE' in params|upper %}
        { action_respond_info(msg) }
    {% else %}
        { action_raise_error('Must provide MESSAGE parameter') }
    {% endif %}

For the code in the python companion, we just need to listen to the gcode-response notification and probably also modify some logic (Since all gCodeResponses are prefixed with //) in the data_sync_service and the in MobilerakerCompanion itself. Let me know what you think about that approach.

Clon1998 commented 1 year ago

Also, by using the actions we can be sure it works with most setups. Your initial macro used the RESPOND command which requires that the [RESPOND] section is included.

TomW1605 commented 1 year ago

yeah that would work, i tried to avoid that because it needs the [RESPOND] module. my macro only used it to make a nice output but if we want to listen to the gcode-response notification then we are relying on that command for sending us the messages. at least thats how i understand it, i dont know if the functions bypass that

TomW1605 commented 1 year ago

up to you what way you want to go, just let me know

Clon1998 commented 1 year ago

up to you what way you want to go, just let me know

I feel like the approach that uses the respond_info action is more reliable since mobileraker would not need to rely on the macro name to match and so on.

If you want I can take over now and implement a POC based on the rewrite branch. But I am a bit busy with work and also the app itself. If you have some time and want to contribute, I would not mind if you take over 😊.

TomW1605 commented 1 year ago

ok ill do a test to see if the action_respond_info functions work without the [RESPOND] module. if it does ill move forward with that

update: i forgot to test this before starting a multi day print, so i wont be working on this till next wee, maybe next weekend

fuzelet commented 1 year ago

I don't have anything to contribute on the coding front but please let me know if you need another tester on this. I have an LCD screen and using custom notifications with a screen has been something I have been chasing myself.

Do I just change to rewrite branch, add the macro, etc?

Clon1998 commented 1 year ago

I don't have anything to contribute on the coding front but please let me know if you need another tester on this. I have an LCD screen and using custom notifications with a screen has been something I have been chasing myself.

Do I just change to rewrite branch, add the macro, etc?

I think @TomW1605 did not create a PR yet. Therefore it is not yet included in the rewrite branch.

TomW1605 commented 1 year ago

yeah sorry, i havnt had a chance to test and move to the action_respond_info method because im doing a long print so cant edit macros. print should be done in a day or so and then ill get it tested and do a PR

TomW1605 commented 1 year ago

so i had a look and action_respond_info does work without the [RESPOND] module. one thing i dont like about this is it means printing the kind of messy message format to the console. since we need extra string processing anyway to handle the // at the start do you think it would be ok to MR_NOTIFY: { TITLE } - { MESSAGE }, then match on // MR_NOTIFY: and -? i can see the - possibly being an issue and would be happy to change that to | if you think thats better

Clon1998 commented 1 year ago

so i had a look and action_respond_info does work without the [RESPOND] module. one thing i dont like about this is it means printing the kind of messy message format to the console. since we need extra string processing anyway to handle the // at the start do you think it would be ok to MR_NOTIFY: { TITLE } - { MESSAGE }, then match on // MR_NOTIFY: and -? i can see the - possibly being an issue and would be happy to change that to | if you think thats better

I'd rather change it to |. I think user's would more often use - than |. Besides that I am fine with the new format. I can take over than and actually implement the matching logic in the mobileraker_companion.py file if you want to.

TomW1605 commented 1 year ago

cool, here is the new macro:

[gcode_macro MR_NOTIFY]
gcode:
  {% if 'MESSAGE' in params|upper %}
    {% if 'TITLE' in params|upper %}
      {% set msg = "MR_NOTIFY: "+params.TITLE+" | "+params.MESSAGE %}
    {%else %}
      {% set msg = "MR_NOTIFY: "+params.MESSAGE %}
    {% endif %}

    { action_respond_info(msg) }
  {%else %}
    { action_raise_error('Must provide MESSAGE parameter') }
  {% endif %}

im a bit worried users might also want to use | as a separator in their message (i often do) so another option is to wrap the message and title in quotes so your looking for " | " and that would be much less likely to clash. note that klipper removes all the quotes from the command when it parses the params so sending message="test" will not result in "test" in the notification when you wrap it

as for the matching logic, im happy to do that but probably wont get to it till the weekend but if you want to do it then go for it. would the changes i made to work with the macro variable method be of any help?

Clon1998 commented 1 year ago

cool, here is the new macro:


[gcode_macro MR_NOTIFY]

gcode:

  {% if 'MESSAGE' in params|upper %}

    {% if 'TITLE' in params|upper %}

      {% set msg = "MR_NOTIFY: "+params.TITLE+" | "+params.MESSAGE %}

    {%else %}

      {% set msg = "MR_NOTIFY: "+params.MESSAGE %}

    {% endif %}

    { action_respond_info(msg) }

  {%else %}

    { action_raise_error('Must provide MESSAGE parameter') }

  {% endif %}

im a bit worried users might also want to use | as a separator in their message (i often do) so another option is to wrap the message and title in quotes so your looking for " | " and that would be much less likely to clash. note that klipper removes all the quotes from the command when it parses the params so sending message="test" will not result in "test" in the notification when you wrap it

as for the matching logic, im happy to do that but probably wont get to it till the weekend but if you want to do it then go for it. would the changes i made to work with the macro variable method be of any help?

Thanks! I will also see how much time I have this week. I think the code in the dataService you provided is not necessary. To add the response info subscriptions will require only 2-3 lines anyway.