st-one-io / node-red-contrib-s7

A Node-RED node to interact with Siemens S7 PLCs
GNU General Public License v3.0
111 stars 58 forks source link

Update to 3.0.0 marks all S7 nodes as having wrong configuration (variables) #83

Closed OriolFM closed 2 years ago

OriolFM commented 3 years ago

Describe the bug

I have a big application in which I read from 30+ Siemens S1200 PLCs using the S7 node, version 2.2.1.

The node-red server is running in our server cluster (intel Xeon machines) with CentOS linux. We have NodeJS version 12.18.3.

When version 3.0.0 came up, I updated it, and after rebooting, every single machine reported errors, saying there were configuration errors in each S7 node.

On closer inspection, the Variables tab marked each and every single address field in red, as if the format was wrong (I hadn't changed the previous one).

I retried deleting a variable and adding it again, but it didn't solve the issue. Note that I have 30+ machines and I read about 20-30 variables from each.

In the end, I had to roll back the server by using yesterday's backup file.

To Reproduce

Steps to reproduce the behavior:

  1. Update S7 to 3.3.0
  2. Reboot the node-red server
  3. See error

Expected behavior

Work as before the update.

Flow

20201208 - U1 Flows.txt

Environment

Additional context

I pasted the current working flow. I had to revert back to the working version quickly because we use the flow for data tracking in our factory. Updating to 3.0.0 and restarting the node-red server will immediately show the error in all S7 nodes.

MrBoggi commented 3 years ago

I also saw the same when I upgraded but not for all variables. What I saw was that I had to change to capital letters for all variable-types, ie. DB600,r4 had to be changed to DB600,R4 for it to work. Maybe this is the case for you as well? I have not looked at your flow, just sharing my experience with the update.

ioT-enthusiast commented 3 years ago

Same problem here. Some variables stopped working, others not.

This worked for me: DB0,X3.0 -> DB1,X3.0

OriolFM commented 3 years ago

I also saw the same when I upgraded but not for all variables. What I saw was that I had to change to capital letters for all variable-types, ie. DB600,r4 had to be changed to DB600,R4 for it to work. Maybe this is the case for you as well? I have not looked at your flow, just sharing my experience with the update.

I thought it might be that because I am not using capital letters, and I tried changing the text in a few variables (not all of the variables in that S7 node). However, I didn't see any change in the ones I changed, and then is when I decided to roll back to the old version.

Same problem here. Some variables stopped working, others not.

This worked for me: DB0,X3.0 -> DB1,X3.0

In my case, it was all the variables. I can't change the DB number, though... the PLC DBs are fixed by the machine vendor, and I have to work with that.

ioT-enthusiast commented 3 years ago

I did not change any configuration in die PLC. Only the adress in Node-Red was changed. The mashine DB0 now reacts to DB1. Strange but works (for me)

OriolFM commented 3 years ago

Glad it worked for you, but the PLCs I use have 300+ DBs... I can't just start trying random numbers for each variable.

As I said, for now, I rolled back and I'm using the old version, but it seems like 3.0.0 is not very stable.

gfcittolin commented 3 years ago

Hi all, thanks for the report.

This new version implements validation of variables, both at the Node-RED's side and at the library's side. While the previous versions would either fail with an unhelpful "bad values" or simply ignore the entry, we're now validating everything and complaining about invalid entries.

@OriolFM in your case, you'll need to change your variable definitions to upper case. From your sample flow, setting everything to upper case cleared all validation issues.

@ioT-enthusiast this is a case where we'd simply ignore it in the past because there's no DB0, so fixing your address list will clear the validation error.

OriolFM commented 3 years ago

This new version implements validation of variables, both at the Node-RED's side and at the library's side. While the previous versions would either fail with an unhelpful "bad values" or simply ignore the entry, we're now validating everything and complaining about invalid entries.

@OriolFM in your case, you'll need to change your variable definitions to upper case. From your sample flow, setting everything to upper case cleared all validation issues.

Thanks for the reply. I understand the need for validation. However, with the old version, everything was working perfectly even with lower case letters. Therefore, I don't see why lower case would be invalid. Then again, it's not my project, so I understand that some languages are case sensitive, and in those, validating both upper and lower case require more work.

Is the error in the node cleared only when I put ALL the variables of the node in upper case, or is it a case by case scenario?

I'm asking because I tried changing some of the variables to upper case in one of the machines, and even after commiting the changes, all the variables were still marked in red. Not sure if I deployed, but for sure I accepted the changes.

Thanks for the good work.

mgdfp commented 3 years ago

@OriolFM : Did you manage to update to 3.0? I tried and got errors, but I do not have any lowercase letters and I am not using DB0. It is working perfect with version 2.1.1. The only message I get is a red triangle on all the nodes and the text "invalid properties: -endpoint" when hovering mouse over. Does anybody know how I can get more information about the problem, logs or such?

OriolFM commented 3 years ago

@mgdfp I did not update to 3.0, I had a lot of S7 nodes and 2.1.1 was working well, so I didn't even try to update, because it's either all or nothing, and spending a whole day changing variable declarations is not high in my priority list.

I'm doing a parallel project in a separate node-red server and I plan on using 3.0. I will use uppercase and see if it works. If so, I will gradually change the ones on the previous project, and eventually update it.

gfcittolin commented 3 years ago

@MrBoggi could you please post both your original flow (before the migration) and a screenshot of the error you're seeing? From the description you've already provided, would be also nice to check the developer console of your browser (can be accessed usually by pressing F12) and check for errors on the console tab

gfcittolin commented 3 years ago

Is the error in the node cleared only when I put ALL the variables of the node in upper case, or is it a case by case scenario?

All of them must be uppercase, and then it must validate correctly. You can use the export button, run through any text processor, and reimport it.

This new requirement for uppercase is for standardization, and for making room for possible future improvements. We want to support other variable naming schemas, and this is in preparation for it. May not be strictly needed, bu we wanted to use the time frame of a major breaking release to include this. Apologizes for the inconvenience.

OriolFM commented 3 years ago

@mgdfp , as I said, I started a new application where I'm using the 3.0 version, and typed all the variable difinitions in uppercase. It is working correctly with the same PLCs, so I assume that was the problem, as it was pointed out. Apparently, it only works if ALL the nodes are configured properly, because if I tried to update some of the machines in my old flow, it won't work at all.

mgdfp commented 3 years ago

@OriolFM , Yes, my mistake, a lowercase "d" was hidden in there and I missed it the first time. I have now changed all definitions to uppercase and 3.0 works fine now.

f00f commented 2 years ago

I think this issue can be closed.

OriolFM commented 2 years ago

I'd say so. I didn't have any more problems.

fernandoamorim1703 commented 2 years ago

We will be closing this issue, if there is a need to revisit the subject, please open a new one. Thank you very much.