thomassorensen2650 / node-red-contrib-mqtt-sparkplug-plus

A node that makes it simple to implement MQTT Sparkplug in Node-Red
21 stars 6 forks source link

Update for { metric.alias: metric.name, ...} #34

Closed archerixx closed 5 months ago

archerixx commented 6 months ago

Hi, would you be able to check this request. I am trying to get metric.name after DCMD is sent towards "mqtt sparkplug device" node. Would you be able to test it out and see if it is possible to make this change ?

Commit note: Save objects { metrics.alias: metric.name, ...} to context at DBIRTH. Then upon receiving payload from DCMD, put back matching metric.name from payload.metrics[...].alias.low before returning payload.

thomassorensen2650 commented 6 months ago

Hi

First of all thanks for creating a PR, its always great to get help :).. I'm not 100% that I understand what problem you are trying to solve.. The MQTT Device will assign and handle aliases internally, without you having to do anything. what is the use case where you need the IDs? and is this something you can't accomplish by issuing a rebirth cmd?

One place where the automatic metrics aliasing is not implemented, is for read data published by other systems using the MQTT Sparkplug In node, but it does not sounds like thats what you are trying to do?

archerixx commented 6 months ago

Hi @thomassorensen2650 , I am sorry for coming back late to you.

My use case is following. I am sending data from external MQTT broker (Currently using Ignition with CirrusLink MQTT Engine) towards MQTT client on Node-Red. Since I am using aliases, when I receive DDATA it comes in this form: {"timestamp":{"low":-479778960,"high":396,"unsigned":true},"metrics":[{"value":true,"type":"Boolean","timestamp":{"low":-479778960,"high":396,"unsigned":true},"alias":{"low":34,"high":0,"unsigned":true},"properties":{}}]} So with that, I do not know which alias belong to which metric.name that I initially sent. And since I only have alias, for example I dont know if that alias is for BatteryStop or BatteryStart command on my end machine.

Currently I am using workaround, where I await DBEARTH certificate and pick up aliases and their metric.name then save them inside context. I attached example of this flow here. flows (64).json

My PR is recommendation to implement this workaround in some way. Where this library would first save metric.name in context upon DBEARTH (aliases ID would serve as key and metric.name as value), then later on upon receiving DDATA from server it would add metric.name in response (instead or beside aliases).

I can try giving some other example if needed. And of course if there is some kind of workaround that I am missing, please let me know.

thomassorensen2650 commented 5 months ago

I like the idea, but I think there is a few things that needs to be fixed and sorted out before we can include.

  1. I don't think we need to store the lookup object in Context. A simple object attribute should be fine (it does not need to be serialized to disk)
  2. There should be an option to enable this functionality using the UI on the MQTT In node.
  3. It should only be possible to enable the option if message type filter is a wildcard/+ (we need DBIRTH, DDEATH and DDATA for this functionality to make sense)
  4. We need unit test to prove that this functionality actually works.

Question: What is the correct way to handle the situation where we get a DDATA package for a device we don't have a birth message from? (this would for example happen if we start node-red after the device). Do we issue a rebirth? and what do we do with the NDATA package until we get a new DBIRTH?

thomassorensen2650 commented 5 months ago

Actually, I just looked at the flow you attached, and I'm not 100% what you are trying to do.

Is the problem you see that when you receive DCMD's that metric aliases are not converted back to metrics names?

thomassorensen2650 commented 5 months ago

Ok, I finally started up Ignition and I can see that this is definitely a bug.. I think the right/simple solution would be to add a small check and convert the alias to names before emitting DCMD. I have a created a small PR #41 that solves the issue without using context.

archerixx commented 5 months ago

Thanks for checking it up and applying fix. I'll try next time to provide usefull PR and tests