phoddie / node-red-mcu

Node-RED for microcontrollers
120 stars 18 forks source link

Inject sensor manifest into plugins build process #71

Closed ralphwetzel closed 1 year ago

ralphwetzel commented 1 year ago

With a small change made to the plugin it's now possible to define - on a per node scope - data that shall be added to the flows manifest.json.

The PR operates on this feature to inject the sensors driver manifest into the plugins build process.

phoddie commented 1 year ago

@ralphwetzel – nice! You inferred one of the motivations for adding the driver path to the sensor database. ;)

I'll get this merged. There's a couple of details I'll adjust after that:

ralphwetzel commented 1 year ago
  • We can always export the includes so that they are available to any tool reading the exported flows JSON. nodered2mcu can strip that from the generated code.

The plugin offers functionalities. As always it's up to the node creator to use these or solve topics differently...

phoddie commented 1 year ago

FYI – I updated as described above. The include array is now on the root of the configuration object rather than _mcu. As before, if not driver is known / needed, the property is not present.

ralphwetzel commented 1 year ago

The include array is now on the root of the configuration object rather than _mcu

Thank you for this update. You need to keep it in _mcu as well ... if you intend that the plugin forwards it to the projects manifest.json.

phoddie commented 1 year ago

Would it be possible for the plug-in to use the include property instead? I'm trying to achieve a couple things:

ralphwetzel commented 1 year ago

Would it be possible for the plug-in to use the include property instead?

That's going to create a dormant error. Any other node creator may decide to name his property as well include, yet use it for different data. The plugin can't distinguish those cases.

I can offer you an alternative though. The plugin emits a signal to indicate nodes that it's preparing a build run. You can assert in this case, that n._mcu is defined. That's how you can use it - in sensor.js:

node.on("mcu:plugin:build:prepare", function(n) {
    n._mcu.include = this.include;
})
phoddie commented 1 year ago

I understand. This is a familiar challenge with interpreting Node-RED exports. Only a handful of properties are defined across all nodes (x, id, name, etc). The meaning of any other property is depends on the type of the node. Event _mcu is not guaranteed to have a single meaning across all nodes. ;)

That's why nodered2mcu interprets such properties based on the node's type. Following that model, the MCU plug-in could maintain a list of nodes that are known to use include with this particular meaning, and ignore it on others.

Using the mcu:plugin:build:prepare event would mean the information is only added when the MCU plug-in requests it. To be generally useful, it should be included in every export. That's also why I don't want to have it depend on the presence of an _mcu property.

ralphwetzel commented 1 year ago

Using the mcu:plugin:build:prepare event would mean the information is only added when the MCU plug-in requests it. To be generally useful, it should be included in every export. That's also why I don't want to have it depend on the presence of an _mcu property.

I understood that it's your intension to avoid duplication and make the include data available to all potential tools. That's how it's currently implemented & fully respected. Reflecting further, I got aware, that there's a second way already implemented that can be used to tell the plugin to inject this data into a manifest - by listening to & utilizing 'mcu:plugin:build:prepare' like shown. Using this interface (additionally) could allow you to maintain your constraints and get the benefit of an automated injection.

phoddie commented 1 year ago

Thank you for your patience. I guess I'm not seeing how this would work when the MCU plug-in is not installed. There would be no _mcu property and, I think, no :plugin:build:prepare event to listen on. Apologies if I'm overlooking something here.

ralphwetzel commented 1 year ago

I was trying to support by introducing you to a functionality that the plugin offers. If you're not convinced that this functionality is a solution to your demands, there's no need to use it...

phoddie commented 1 year ago

Understood & appreciated! I seems our starting design points are different. You expect the MCU plug-in to aways be present. I am trying to design the nodes to export the same information regardless of the plug-in's presence. I'm glad we sorted that out.

ralphwetzel commented 1 year ago

I'm glad we sorted that out.

Thank you for explaining once again your POV.

You expect the MCU plug-in to aways be present.

Not at all. I was proposing a three-line-addition to solve the discussed issue in case the plugin is present.

I am trying to design the nodes to export the same information regardless of the plug-in's presence.

That's what I understood already. Fair intention...

Sineos commented 1 year ago

Maybe I do not understand all associated implication but for me as a user, I'll almost 100% stick to using the plugin. While this flexibility of the node export without the plugin surely is a valid goal, the question is how many will use it.

If I could do all this stuff singlehanded in JS I'd not even use Node Red to program a MCU. Since I can't, I would sort myself into exactly the target group of the plugin and everything what is not working "out of the box" is a stumbling block in the first place.

In any case I very much enjoy playing around with these products and hope you find a solution for a seamless integration.

phoddie commented 1 year ago

While this flexibility of the node export without the plugin surely is a valid goal, the question is how many will use it.

Thank you for the feedback and perspective.

I believe it is important that the export should be consistent for a given node. That puts all tools that operate on the exported data on equal footing. That is important for my development and testing, even if I am in the minority. ;) It also keeps the implementation of the nodes simple and predictable.

I understand the primary objection of @ralphwetzel to what has been implemented is that the include property name is insufficiently unique that it could potentially cause conflicts with some other nodes. A simple way to address that valid concern would be to change to a sufficiently unique property name.

Sineos commented 1 year ago

the include property name is insufficiently unique

Just a stupid idea: Could moddable define an alias to include, e.g. mcu_include, that is parsed the same as the original include? Then such plugins could resort to mcu_include while preventing a breaking change in the moddable environment.

phoddie commented 1 year ago

Thanks for the suggestion. I don't think believe anything is proposing a change to the manifest file format itself. What is being discussed is the JSON created by exporting the MCU Sensor node from the Node-RED Editor. Here's an abbreviated version of what that looks like, using include.

[
    {
        "id": "6adc05ece2bc0f1e",
        "type": "sensor",
        "module": "embedded:sensor/Touch/FT6x06",
        "include": "$(MODDABLE)/modules/drivers/sensors/ft6206/manifest.json",
        "_mcu": {
            "mcu": false
        },
        "wires": [
            []
        ]
    }
]

As noted above, @ralphwetzel objects to include as the property name as it has the potential to conflict with other nodes in the Node-RED ecosystem. That's a fair concern. To address it, it seems sufficient to simply use a more unique name. Here's one example (many other names could work too):

[
    {
        "id": "6adc05ece2bc0f1e",
        "type": "sensor",
        "module": "embedded:sensor/Touch/FT6x06",
        "moddableManifestInclude": "$(MODDABLE)/modules/drivers/sensors/ft6206/manifest.json",
        "_mcu": {
            "mcu": false
        },
        "wires": [
            []
        ]
    }
]

If we imagine that in the future there might be other information to include that is specific to the Moddable SDK, we could do something like this:

[
    {
        "id": "6adc05ece2bc0f1e",
        "type": "sensor",
        "module": "embedded:sensor/Touch/FT6x06",
        "moddable": {
             "include": "$(MODDABLE)/modules/drivers/sensors/ft6206/manifest.json"
        },
        "_mcu": {
            "mcu": false
        },
        "wires": [
            []
        ]
    }
]

My understanding is that Ralph proposes instead to put the information in the _mcu element owned and managed by the MCU plug-in:

[
    {
        "id": "6adc05ece2bc0f1e",
        "type": "sensor",
        "module": "embedded:sensor/Touch/FT6x06",
        "_mcu": {
            "mcu": false
           "include": "$(MODDABLE)/modules/drivers/sensors/ft6206/manifest.json",
        },
        "wires": [
            []
        ]
    }
]

Anyway. That's my summary of the situation. If that's incorrect in some way, I'm happy to be corrected.