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

Update to phpmqtt_input to accept JSON object #776

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@borpin
Copy link
Contributor

borpin commented Jan 13, 2018

Not done much PHP work so all comments gratefully received

Modified phpmqtt_input to accept a validated JSON name:value object. Used the InputMethods class to actually inject the data.

Updated some of the indenting as it did not line up (helped me work out what was what).

.gitnore updated to ignore VSCode folder.

TODO - look for a 'time' name in the input data, put it into $time and remove from JSON object. (help with this appreciated from an expert in PHP - I'll work it out eventually though!).

TODO - update comments / help

Update to phpmqtt_input to accept JSON object
Modified phpmqtt_input to accept a validated JSON name:value object.

TODO - look for a 'time' name, put it into $time and remove from JSON object.

TODO - update comments

.gitnore updated to ignore VSCode setting folder.
@TrystanLea

This comment has been minimized.

Copy link
Member

TrystanLea commented Jan 13, 2018

Thanks @borpin I will look over your code and test here in the coming week

borpin added some commits Jan 13, 2018

Update to allow the use of non numeric dates in the past
A problem was found in that emoncms does not handle non numeric dates in the past elsewhere.  This converts the date to a number of seconds.
@borpin

This comment has been minimized.

Copy link
Contributor

borpin commented Jan 13, 2018

Latest commit relates to issue #777 and allows a 'time' value to be used when processing the JSON message.

Currently the time data is not deleted from the input data.

@TrystanLea

This comment has been minimized.

Copy link
Member

TrystanLea commented Feb 8, 2018

Sorry for the delay @borpin. I've had a good look through the pull request now, there was a lot in there to take in at once. I see your using the 'process_node' function rather than passing the data to the lower down processing stage in the script, do you think you would be able to use the in-built processing stage instead, was there a reason for this decision?

@TrystanLea

This comment has been minimized.

Copy link
Member

TrystanLea commented Feb 8, 2018

Is it worth also having a discussion re accepted formats? I know @pb66 bought this up in the past. So far we support

basetopic/emontx/power 10
basetopic/emontx 100,200,300

your suggestion adds (if I read it correctly?):

basetopic/emontx {"power1":100,"power2":200,"power3":300}

and the option to set a time:

basetopic/emontx {"time":UNIXTIME, "power1":100, "power2":200, "power3":300}

Should we also support?

basetopic {"nodeid":"emontx", "time":UNIXTIME, "power1":100, "power2":200, "power3":300}

and even:

basetopic [
    {"nodeid":"emontx", "time":UNIXTIME, "power1":100, "power2":200, "power3":300}.
    {"nodeid":"emontx", "time":UNIXTIME, "power1":100, "power2":200, "power3":300}
]
@borpin

This comment has been minimized.

Copy link
Contributor

borpin commented Feb 8, 2018

To be more accurate rather than it being emontx it is

basetopic/nodeid {"time":UNIXTIME, "power1":100, "power2":200, "power3":300}

so I do not see any benefit of concatenating the data further as this reduces the MQTT connections significantly and it is rare that more than one node reports at exactly the same time I'd suggest. It is also usually true that a node will report all the data elements in one go (although I currently have 2 x CO2 sensors reporting separately and independently into the same node which breaks the paradigm of node = HW device).

I see your using the 'process_node' function

That was because I simply used the same method the HTTP API employed 😄

$result = $this->process_node($userid,$time,$nodeid,$inputs);

Before the PR is merged, I will

  • remove the case sensitivity of 'time' key
  • add an INFO message re the time actually used so it should be more obvious if the setting of the time fails.
@TrystanLea

This comment has been minimized.

Copy link
Member

TrystanLea commented Feb 8, 2018

I've created a development branch for these changes called mqtt_input_json and have merged in your changes there. I've got a couple of modifications I would like to make and I think this is probably the easiest way to do that as I could not seem to send a pull request to your fork
87f7669

@TrystanLea

This comment has been minimized.

Copy link
Member

TrystanLea commented Feb 8, 2018

ok this is probably a bit hard to follow, the git compare is not particularly clear. This is what I meant by using the inbuilt processing rather than input methods
https://github.com/emoncms/emoncms/pull/788/files
It results in less lines added overall and avoids having two processing routes

@borpin

This comment has been minimized.

Copy link
Contributor

borpin commented Feb 8, 2018

It results in less lines added overall and avoids having two processing routes

Achieves the same end.

I could not seem to send a pull request to your fork

I'd need to give you access I think; no actually I have no idea. I find Git impenetrable!

I was going to make some minor changes to the logging around setting the time so it was clear what had happened but I now can't work out how to! May be too much info for the log although there is no other way to be sure the time set has been taken.

                    } else {
                        //Do nothings as it has been assigned to $time as a value
                        $log->info("Valid time string used".$time);
                    }
                } else {
                    $log->info("No time element found in JSON - System time used");
                    $time = time();
                }
            } else {
                $jsoninput = false;
                $log->info("No JSON found - System time used");
                $time = time();
            }
@borpin

This comment has been minimized.

Copy link
Contributor

borpin commented Feb 8, 2018

I added the log bits in via another PR #789 but I suspect that may have really confused things!

@borpin

This comment has been minimized.

Copy link
Contributor

borpin commented Feb 8, 2018

I have added in making the search for the time key case insensitive.

I also realised that the case where there is a time string but it is not valid did not result in a time value being set.

@TrystanLea

This comment has been minimized.

Copy link
Member

TrystanLea commented Feb 8, 2018

"Achieves the same end." yes certainly, I've merged my suggested changes in #788

@borpin

This comment has been minimized.

Copy link
Contributor

borpin commented Feb 8, 2018

Just created a PR #790. It crashes the mqtt_input service as it stands.

@borpin

This comment has been minimized.

Copy link
Contributor

borpin commented Feb 8, 2018

Changes commited via #788 so closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment