#935 PLCgen: CifProcessor collects results, target distributes them.

changed milestone to %v5.0

PS I thought we could have empty lines before 'else if' branches, but the formatter killed them again. Was I wrong in thinking that?

Do I see it correctly that this partially replaces !1028 (closed)? For now, I'll assume so. I'll review this and for now I won't review !1028 (closed).

I started !1028 (closed) as an experiment to see how it would work out. It worked out much better than I expected.
So I thought it would be a good start at least and pushed it, except I didn't start from the correct revision apparently.

Usually revision differences resolve themselves, but this patch is making large code moves and changes, touching many files, and I am not even close to "done" here. Conflicts are always messy to resolve, and here even more due to the scale of changes. I looked at the conflicts briefly, but considered the risk of making errors in resolving too large.

The second reason for not continuing in the !1028 (closed) direction now, is time. In any case this will take a few weeks and I am not sure we have that time now. So while the direction looks good to me, I moved priority more towards having functions for event transition processing. It's going to be less nice than with a central program managing the steps, but eventually that should resolve itself while making progress in the direction that I started with !1028 (closed).

I think introducing functions has a bit less prio now. For v5.0 we made enough improvements already, and RWS can continue. I think the other bugs that are still open in #679 should maybe get the highest prio now. It would be great if the self-certification test set could me made to succeed.

Huh? We had

Too much code, as discussed at #912 (comment 2545340)
Too much data, as discussed at #912 (comment 2546137)

We worked on reducing the amount of data only afaik.

Right, I forgot about that. I thought that was also to split the variables over multiple files.

That problem exists too, as a result of moving code. Obviously, if you move code, you also need to move variables that it uses.

Now, if one piece of moved code is the only user of a variable that's easy. However, we have generally used "edge" and "old value" variables. Those will be needed in many functions. As far as I can see just copying them should work, as different functions have separate scopes and we don't use variable identity in the java implementation (unlike eg in a meta-model).

Do you mean that you can want to declare the variables once and just refer to the same variable in different places, copying them to multiple tables? That will break, as different sub scopes have different name generators. I think we must really declare the variables per scope, generates names for them per scope, etc. But, maybe that is what you meant as well.

I was thinking the former, for edge variables. It's useful to have the same name for the same variable everywhere, since the model view comment text shows them as part of automaton variables, so different names for them in different functions is bad.
Also, the isEventEnabled and the isProgress variables are commonly used although not documented, except in code-level comment. I'd expect that the comment is generated is using the name of the variable itself, but not sure about that.
I am thinking to make the isProgress an input/output parameter, so the function updates on progress and it solves the "needs at least 1 parameter" limitation on functions.

Maybe the edge variable names should be declared in global name space? The isEventEnabled and isProgress too, except I think they are already global.
Then the names are not available for local scope use. Creating one such variable and copying it into different local variable tables will work I think. While the variable is then shared in Java, the writers don't use that fact at all as far as I know.

Other variables are likely fine. State variables and current locations are global already. Temporary variables in functions for computing expressions are indeed local and must have a new expression generator for each function.

I'm not sure I like the copying of the variables. Are we sure there are no differences for references depending on where the variable lives?

Temp variable in the main program versus temp variable in a function makes no difference currently, but I agree it's not entirely clean.

I just tried creating the names directly with the name generator rather than as temp variables through the global expression generator. While the variables are created and used, they are not added to the main program as temporary variables (since the transition code lives in that scope currently).
It's a step forward in disconnecting the variables from the main function, but the proper solution seems to be

Create edge variable names as global names (so they are the same everywhere). Keep the automaton and the name together (probably a Map<Automaton, String>).
Have the owner of the function POU that will contain the transition code create variables from the names (for the subset of of edge variables that is used in the function).
Add them as temp variables to the function POU.
Tell the transition generator to use those variables as edge variables.

Currently the owner of the main program is CodeStorage, and the POU is quite buried iirc. I'll have a look at it tomorrow.

changed title from #935 (closed) CifProcessor collects results, target distributes them. to #935 (closed) PLCgen: CifProcessor collects results, target distributes them.

I added 'PLCgen' to the title.

requested changes

changed the description

I added the end-user visible changes to the merge request description.

added 1 commit

a6e15463 - #935 (closed) Apply suggestions.

Compare with previous version

added 1 commit

be0b9ded - #935 (closed) Check having a specification while creating the CIF object finder.

Compare with previous version

all done

approved this merge request

merged

#935 PLCgen: CifProcessor collects results, target distributes them.

Merge request reports

Activity