I’m baffled by your error-reproduction-abilities^^
I did not anticipate this at all and hope this did the trick now
Well I had written a substantial post going through all my test steps but @Adminde has just posted this PR
Which has cleared up the loose ends, It now works.
There are several nasty consequences of having to flush redis which I’ll post on later.
But for now this commit has fixed the missing proceslists (i tested it fully before merging) but now I have pulled this fix into my live repo, switched branches to the “device-integration” branch and re-run the init and the missing processlist has been populated. Hooorah! Thanks @Adminde.
[edit]
I am now unsure whether to roll-back to avoid any new nasty surprises or to stick with this branch, it would seem the master is currently as uncertain as the device-integration.
I have tried using the master branch and it broke again, after lots more tests I have established that the device master branch will not work with 9.8.11, so we are stuck with using the “device-integration” branch only with 9.8.11 and that won’t work with any versions earlier than 9.8.11 (tested with both 9.8.8 and 9.8.10). so there appear to still be some backwards compatibility issues with the device-integration branch of the device module but I have not yet tried this PR Backwards compatibility and fix feed tag verification by adminde · Pull Request #10 · emoncms/device · GitHub hopefully that fixes that branch.
Is the “set_processlist-patch” branch purely a “master branch for use with just v9.8.11” ? I haven’t tried it yet and I’m getting quite lost in the different combinations.
So are we saying any users that apply the security fix (or any subsequent update) to emoncms will break the device module if it’s installed, unless they also switch the device module branch? How will they know? The issues caused cannot be fixed/reversed by installing the “set_processlist-patch” after the event, installing the “deice-integration” branch might, but that clearly needs further testing before being recommended.
Are we not able to get the master branch to work with ALL recent emoncms versions before testing the “device integration” branch? If the changes in “set_processlist-patch” work, the “device integration” branch can be rebased can’t it?
I have also noticed that the group tags defined in my template.json were not honoured, all the feeds have been created in one group which is named after the node. My template defined 4 different groups.
Previously, the ability to define (multiple) tag names was great to have, but with the new ability to re-initialize devices it is essential. If I was to now correct the fact the feeds are not grouped by 4 tags and need/want to re-initialize the device, all the feeds will be duplicated, this behavior basically removes any free-form sorting in emoncms, users will not have any way of personalizing the way feeds are grouped. It also removes any chance of applying any cross node totaling in the device templates.
I have returned to this briefly to try and fathom out the current state of play so that I know where I am when I next need the services of device module.
So when I started out my test server was running 9.8.20(master) and the “device-integration” branch of the device module. Based on the fact I cannot go back to the master branch of the device module because it will only run on emoncms <9.8.10(ish, haven’t confirmed the break point, it will work with 9.9.8 but not 9.8.11) I decided to try the set_processlist_patch branch to see if the feed group tagging worked since my last experience with the device-integration branch resulted in
So I switched device module branches from device-integration to set_processlist_patch (ran a pull just in case) and deleted all the redis keys with a emoncms:device: prefix, I also cleared browser cache. IMO I am now running a fully up to date emoncms and device module (excluding any device-integration dev). It certainly looks like the original device module.
To test the tag grouping I copied the latest emonth template and edited it to be called “test” and added "tag": "mytag" to the first feed created.
{
"name": "test",
"category": "OpenEnergyMonitor",
"group": "Temperature & Humidity",
"description": "Automatic inputs and feeds creation for emonTH device.",
"inputs": [
{
"name": "temperature",
"description": "Temperature C",
"processList": [
{
"process": "1",
"arguments": {"type": "ProcessArg::FEEDID", "value": "emonth_temperature" }
}
]
},
{
"name": "humidity",
"description": "Humidity Rh%",
"processList": [
{
"process": "1",
"arguments": {"type": "ProcessArg::FEEDID", "value": "emonth_humidity" }
}
]
}
],
"feeds": [
{
"name": "emonth_temperature",
"type": "DataType::REALTIME",
"engine": "Engine::PHPFINA",
"interval": "60",
"tag": "mytag"
},
{
"name": "emonth_humidity",
"type": "DataType::REALTIME",
"engine": "Engine::PHPFINA",
"interval": "60"
}
]
}
When I created a device based on this template I was pleased to see the tag was honored and the 2 feeds were created in 2 groups (“mytag” and “NewNode”), this is great, I am now fairly confident I can create the devices I’ll need when required.
Partially to confirm my findings the other day and partially wanting to try the new “multi-init” part of the new “device-integration”, I then proceeded to change to the “device-integration” branch (and pulled again, just in case) and cleared the browser cache.
On the device page the device fields were all “undefined” due to the redis being flushed. at this point there is no way to get the fields populated without creating another device, so I created a redundant dummy device just so I could resume testing on the existing device.
From the device page I re-init’d the same device expecting a new “emonth_temperature” feed to be created in the “new Node” group as I expected the '“tag”: “mytag”` to be ignored in favor of the device name “New Node” based on my previous experience and this comment
However, nothing changed. In case this was some weird redis caching or something I edited the “emonth_temperature” feed in the template to "tag": "mytag2" to cause a new feed to be created, that did not happen.
So in the feeds page I edited the “emonth_temperature” feed to be in group “mytag3” (from mytag), returned to the device page and re-init’d again, still no change, it has not created a “emonth_temperature” feed in either “mytag2” or in “New Node”.
I cannot tell at this stage if it is ignoring the tag groups all together or if it’s not actually doing any initializing at all so I deleted the “emonth_humidity” feed altogether and re-run init again, this time it created a new “emonth_humidity” feed in “New Node”.
So it would seem that it is not a tag+feedname combination, but feedname only that is used AFAICT, although confident in my findings, I am not confident this isn’t some weird redis handing issue.
I did try deleting the redis key for the “test” template and re-init’d again, but it still did not create a “emonth_temperature” feed in group “mytag2”, I checked the redis keys immediately after running this and the"test" template key was back as expected.
Last test, I deleted the one “emonth_temperature” feed (currently in “mytag3”) and ran the init again, as expected it created the “emonth_temperature” feed in group “mytag2”. It certainly seems the device module is ignoring any feed tags except when creating a feed with a unique feed name, at which time it does seem to honor the defined tag in the template, why this wasn’t the behavior I saw the other day I have no idea!!!
As it stands I expect any flushing of redis to cause a situation where the device page is not populated and devices cannot be initialised etc until another device is created, there needs to be a trigger to fetch data when not found in redis.
It also seems the feed tagging is an issue that needs some work. I do not think we can have the ability to re-init without full tag:feed name indexing as this would hamper any use of duplicate feed names, effectively you can only have one emonTH, subsequent emonTH’s maybe created but they will all be writing to the same 2 feeds. I hope I’m wrong on this point, but if I am it only highlights other problems as that is definitely what I’m seeing here today, but yes that does differ from what I saw the other day so who knows?
To answer a question from much earlier in this thread
IMO no definitely not, if anything we need more grouping. It a shame the feed tags are a single level index. I am in no way suggesting we do this, but to explain the functionality it would be nice to have multiple tag fields so that we could (on the same page) switch grouping between location, phase, feed type, device, favorites, sub-total feeds v individual feeds etc etc.
I see the device page now has grouping by location (in the device-integration branch) which is cool, when managing several devices it is useful to group by location. The feed tags though help organize feed lists in the various places like widget drop downs and the input processing editing etc. Removing the feed tags and adding device location would be a big step backwards (especially for those not using the device module), assuming all the feed dropdowns would eventually move over to “location grouping” (or worse -
unsorted lists) we will have simply swapped tag:feed for location:feed and we will all start falsifying the “location” to suit our individual grouping demands.
Much like here on the forum where categories are equal to the feed’s tags, it would be nice to have the equivalent of the forum’s tags for feeds, but I get that is complex and do not expect to see anything like that, anytime soon, however acceptance of that doesn’t mean I would like to lose the current feed grouping.
Whilst re-reading this thread, I noticed this in the first post
and no mention of this behavior changing, so is there any actual changes to the way feeds and feeds+tag are used in the device-integration branch or has this behavior just been carried forward from the master branch?
[edit]
@TrystanLea and @Adminde Sorry for the really long post (even by my standards 
). but if you have read it all, well done and thank you.
[edit2]
just noticed @Adminde’s “Backwards compatibility and fix feed tag verification” PR still hasn’t been merged, so the “current” behavior may have changed again. Is there any reason this isn’t merged @TrystanLea?
thanks @pb66 I missed @Adminde pull request, I was just going through your post and the code and noticed the missing:
$feedid = $feed->exists_tag_name($userid, $name);
check which I see @Adminde has fixed great!
I’ve found a couple more issues, just working through them
Edit: you can see the missing tag in the line I quoted 
Ok I’ve pushed up the fix.
- Now, if I rename the tag on emonth_temperature to mytag3 it creates a new feed under tag=mytag
- and rewrites the processlist to point to the new feed
Heads up! another long post!
Just pulled in the changes and tried to re-init and existing device that I have since edited the template for so that emonth_temperature has tag:mytag3, so starting with feeds
| tag | feed name |
|---|---|
| New Node | emonth_humidity |
| mytag | emonth_temperature |
I ran the init for my same “New Node” device again and was surprised to see I now have
| tag | feed name |
|---|---|
| New Node | emonth_humidity |
| mytag | emonth_temperature |
| mytag3 | emonth_temperature |
| testing | emonth_humidity |
So the additional “emonth_temperature” feed was created with a different tag in this instance BUT where did the “emonth_humidity” feed with the testing tag come from? Well, if you recall I created a throw away device to get redis to populate (except I didn’t throw it away), I have never initialised this device!
Not believing my eyes I edited the test template again to include “mytag4” a previously unused tag ref and then I created a new device called “test2” (I now have 2 virgin devices that have never been initialised “testing” and “test2”) and a revised and unused template for an existing device. No (re)initialisation has happened yet in this test.
To make sure of the results of my test I revisited the feeds page at this point to confirm the “before” immediately before re-initializing my “New Node”. You won’t guess what I found!
| tag | feed name |
|---|---|
| New Node | emonth_humidity |
| mytag | emonth_temperature |
| mytag3 | emonth_temperature |
| testing | emonth_humidity |
| mytag4 | emonth_temperature |
| test2 | emonth_humidity |
I have not ever initialized test2 or re-initialised the “new Node” since editing the template !!!
As for why only the “emonth_humidity” is created for “test2” I have no idea…
[edit] you have posted whilst I was writing so it maybe you’ve found or addresses some of this.
aha the devices are initialising when you click save in ‘configure device’
I’ll need time to digest this…
What if the processlist has been edited, changing a feed tag in a template losses the changes made to a processlist. I guess you would want the link between the newly tagged feed and the processlist to be retained though. This also touches on what I was saying about having the ability to define a tag+feed name in the process list, if I was to edit the feed tag in the template’s feed definition AND in the template’s process list, the link to the processlist would still be correct and there would be no need to overwrite the processlist unnecessarily, but currently we can only always ignore or always overwrite the processlist I guess, there is no easy way to retain externally edited processlists.
Thanks for testing Paul, I think as a first step I will add a initialisation log output to the UI so that we can actually see whats happening.
That sounds like a great idea, I think it would be useful if it was retained for future debugging, perhaps make it collapsible so it’s generally hidden.
Ok, I’ve created an initial version of a device initialisation log, I think it should not be too hard to adapt this to include a dry run option
Looks great!
Just tried it, twice on the same device to ensure there were no changes to be made and got
which suggests to me the processlists are being over written even if there are no changes.
Couple of very minor formating points…
Can the feed:tag be swapped to tag:feed so that in matches the node:input, group:item convention?
Could “set processlist” be just “SET” to match “CREATE” and “EXISTS”?
Maybe we could use a pipe or something else rather than a colon for group:item so that “set processlist inputid=73 processes=1:65” could simply be “SET 73 | 1:65” since it is already under the sub-heading input processes,
Perhaps it should also use the input name rather than inputid as that doesn’t relate to anything in the context of the device in hand. Likewise for the feedids perhaps?
Also
Where are we with respect to using the process name rather than number? (ref Processes only accessible via the depreciated mechanism · Issue #6 · emoncms/device · GitHub) it would be good to make the json templates 100% readable.
That sounds great too!
Just noticed you have pushed some more changes, I am just about to shoot out for while so I haven’t looked at what they are yet. maybe you some of this covered already.
I’ve made most of the adjustments as you suggest. The device templates support input process names, I realised I missed updating the emonth example!, the emonpi-SPV2 template is a good example.
Looks a little better, am I right in thinking the processlist is being overwritten regardless? I assume you have made the messaging the same and it’s not incorrectly reported?
great!
How did you implement making the device module work with the original “numeric id only” typr processes? Did you do it in the device module with a lookup or have you done it in emoncms core?
I was going to suggest we replicate the original “numeric id only” processes in a seperate module(within emoncms) and hide the originals from any drop downs in the input processing so that over time the numeric process ids get replaced by descriptive names so that we can move towards depreciating the ids altogether one day down the line.
I think this new debugging output will help no end whilst developing the device module, good work!
I have pulled in the latest and checked the template has updated to use “log_to_feed” rather than “1” and re-run the init a couple of times as I wasn’t sure if the message output read from the template or the device config, but I’m still getting the numeric processids in the messaging (I guess that answers my question above, the conversion is done in the device module). I now have this
feeds:
-- EXISTS mytag4:emonth_temperature
-- EXISTS New Node:emonth_humidity
inputs:
-- EXISTS New Node:temperature
-- EXISTS New Node:humidity
input processes:
-- SET New Node:temperature
1:65
-- SET New Node:humidity
1:64
feed processes:
previously I was going to suggest dropping the \n in the center of device_template.php line 186 so that the output is all on one line like the feeds and inputs, but I can see that getting way out of control, especially when using long process names (and possibly tag:feed) instead of ids.
Now I think it would be better to split the processlist on the comma’s (is it a string or a list?) and do a “for each” so that each process is on a new line, more in keeping with the template format, perhaps with an indent like so
feeds:
-- EXISTS mytag4|emonth_temperature
-- EXISTS New Node|emonth_humidity
inputs:
-- EXISTS New Node|temperature
-- EXISTS New Node|humidity
input processes:
-- SET New Node|temperature
log_to_feed:mytag4|emonth_temperature
-- SET New Node|humidity
log_to_feed1:New Node|emonth_humidity
I’m not hung up on using the pipes but I do think we need to avoid using the colons because the processlists already use that as a separator, so we cannot use it again as a sub-separator if tag:feed ever gets used in the processlists. It looks like all the group:item dividers are hardcoded in so it’s just a “typo” type change.
besides every time I write “tag:feed” here on the forum I get offered a breast feeding emoji…
A little more work on this:
- Removed inline editor as editing is available through dialog
- use of mysql prepared statements in device_model
- process list log with process name and feed/input names
- initialisation check when the device is created
- re-initialisation button visible when device type is not changed
- option to place device templates in sub-folders
Edit: If you get an empty init log after a redis flush, there is a bug fix in the emoncms master branch for this
Can you add a field for the devicekey please?
I’m not sure I understand why you would rather remove the established colon from each processlist item rather than not start using a colon in a group:item ref.
It seems the device module is basically taking the process name, converting it to a depreciated process id, saving the processlist in the depreciated and un-human friendly format and then converting the id to a process name in the message log so it looks like it is working with process names. I thought the idea of the log is to see what IS happening not to make it look like something else is happening.
All my inputs are still using process ids
the original device module is capable of producing text based processlists, except for the original id based processes here is a couple of experimental inputs of mine that used an additional drop in lisy of processes.
do you not think this is a lot of processing to use the id’s when names could be used throughout and not require any conversion or reconverting back just to display in the log?
do you mean “initialization” button? I haven’t seen a re-initialization button.
I’m not sure that is intuitive, I am cancelling the init when saving a device. IMO save and init should be separate and permanent buttons so you know what you are doing/getting.
Great.
It still “appears” the processlists are always overwritten, Is this because the templates process names are not equal to the saved process ids so it thinks there are changes to be made?
I’ve added in the ability to set a new devicekey. Rather than provide a text box to enter a devicekey its just a ‘new’ button that generates another key.
While the device templates files now accept human readable process names, these are converted to process id’s and input id’s during the device initialisation process. Hence processlist=1:65 is the reality. I have thought about the possibility dropping the use of inputids as the combination of userid, nodeid and input name defines a unique input without the need for an additional id field but this is something for future development.
The processing to produce the log doesnt really matter, the more important consideration would be the performance of the input processing process which happens every time a device posts data. Doing a string match should be slower than an id based lookup, whether its significant on a busy server like emoncms.org would be something that would need looking into.
yes
The initialisation popup only appears when your creating the device for the first time or changing the device template. If you just save a change of name or location it does not ask again at that point.
Yes processlists are always overwritten in the current implementation
Edit: I’ve changed the colons to vertical line characters