pnp-software / fcp-editor

Web-based editor for specifying Flight Control Procedures (FCPs) based on ESA's SCOS-2000 standard.
MIT License
3 stars 1 forks source link

Syntactical Check #14

Closed pasetti closed 2 years ago

pasetti commented 3 years ago

In the FW Profile Editor, in the panel to the left of the screen, one choose the "generate code" option. This option is not needed for the FCP Editor where there is no code generation but I am thinking of using this button to perform a "syntax check". I am still not sure what kind of checks should be done here but the general idea would be to verify that all links which have been introduced with the auto-completion feature are valid.

Tomas-M commented 3 years ago

Dummy function implemented in 5a10afcfb391e8f2c4031461ae2a2c7af16df00b

pasetti commented 2 years ago

The model created by the FCP Editor will probably need to be processed by a script to create an executable test sequence or flight procedure. In order to facilitate the development of such script, it is useful to define some rules on how the model should be built and it is useful to have these rules verified by the "FCP Editor Check" (second icon from the top in the banner at the left of the screen). In this ticket, I define a first set of rules. More may be added in the future. For traceability, the identifier of the rule should be put in the javascript code which implements the rule.

Rule 1 All references to SCOS items in the text fields of the model (i.e. all strings starting with: "#TC(...", or "#TM(,...", or "#HK:...", or "#EID:...", or "TMPAR:...", or "TCPAR:...") must be valid with respect to the SCOS database selected for the model.

Rule 2 It is not legal for the Action Description of a node to contain more than one reference to a telecommand "TC(..."

Rule 3 If the Action Description of a node contains a reference to a telecommand "TC(...", it cannot contain references to any other SCOS elements

Rule 4 If the Action Descriotion of a node contains a reference to a telecommand "TC(...", then the Notes field attached to that node can only contain #TCPAR references which belong to that telecommand. This rule is illustrated in the screenshot below where we see NODE1 which makes a reference to telecommand SET_CSS_CURR_CONFIG and its Notes field which holds two TCPAR references to telecommand parameters which are in that telecommand image

Rule 5 It is not legal for the Guard Description of a control flow to hold a reference to a telecommand "#TC..."

Rule 6 It is not legal for the Action Description or the Guard Description to contain more than one Event Packet reference "#EID:..."

Rule 7 If an Action Description or a Guard Description contains a reference to an Event Packet "#EID:...", then it cannot hold any other SCOS reference

Rule 8 If an Action Description or a Guard Description contains a reference to one or more housekeeping parameters "#HK:...", then the only other SCOS reference it may contain is one telemetry packet reference "TM(..." and that packet must contain all the referenced HK parameters. Note that a housekeeping parameter may be contained in several telemetry packets. This rule is illustrated in the figure below which shows a node which references two HK parameters and one HK packet. The two HK parameters must be in the same TM packet.

Screenshot from 2022-03-20 09-42-11

Notification of Check Failure If a check fails, the checker should output a message with a brief description of the failure and and, if possible, some indication of where in the model the error was found (e.g. the name of the node to which the text field with the error belongs)

Tomas-M commented 2 years ago

I think I understood everything and I can start on this tomorrow (monday). I do not expect this to take longer than a hour or so, seems straightforward. I will let you know if I encounter uncertainties

Tomas-M commented 2 years ago

Lets assume that the current model has so many flaws, that each rule is broken. This would lead to potentially very long list of problems after the Check button is clicked, which may be hard to navigate. Do we want to display all the errors (all broken rules) at once? Or should it go through the rules one by one, and if a rule is broken, then it lists that particular rule is broken (and shows where) and then it stops, so no more other rules are checked, until the errors for previous rules are fixed?

I think it may have advantages (shorter list of errors is easier to navigate) as well as diadvantages (user does not see all errors at once). So it is probably up to you to choose :) Let me know please what do you prefer.

pasetti commented 2 years ago

I think that listing broken rules one-by-one (or maybe two-by-two and maybe followed by something like "..." to inform the user that there are other errors).

Tomas-M commented 2 years ago

I noticed a possible bug or problem. When we autocomplete #EID, the editor offers syntax with a bracket, such as #EID(4,3) But one feature which checks for invalid strings considers syntax without bracket but with a colon, such as #EID: What should be used? EID with bracket or colon? You also mentioned #EID: in this issue. Only one of the two should be used in the editor to work properly.

pasetti commented 2 years ago

The correct syntax if: "#EID(x,y):...". Apologies for the confusion ...

Tomas-M commented 2 years ago

For rule 4: Notes field attached to that node can only contain #TCPAR references which belong to that telecommand - does this mean that the Notes field can contain only #TCPAR references? Or does it mean it can contain some other references, but those ones which are #TCPAR must obey the rule (belong to TC in Action Description)?

pasetti commented 2 years ago

The intended meaning is that the Notes field can contain only contain #TCPAR references and no other kinds of SCOS references (I will make this point clearer in the final version of the rules)

Tomas-M commented 2 years ago

Thanks. Also, it seems to me that Rules 2 and Rules 3 are overlapping (actually, Rule 3 matches also errors from rule 2) since for example #TC(1,2)... is a "different" reference than #TC(3,4)... so it will match also as "other SCOS element". So I suggest to delete rule 2

pasetti commented 2 years ago

Yes, you might be right ... I will need to re-write the rules to make them clearer. If you see other ambiguities, do let me know!

Tomas-M commented 2 years ago

I would say the same applies for rule 6 and 7

Tomas-M commented 2 years ago

I've pushed the changes, please test. f7a5a2bd5ee8170effdddeaa73da7b8900b21487 Commented out are rules 2 and 6 since those are catched by 3 and 7 respectively.

This commit also fixes a bug in main, where EID: was searched for when checking for SCOS version compatibility (in tools) instead of EID(...) so it probably didnt catch incompatible EID: references

pasetti commented 2 years ago

I have checked the implementation of the rules and they seem to be correct. I only have one comment: if the checker finds an error, it displays an error message with a description of the violated rule and a pointer to the node where the violation occurred. If, however, the checker does not find any violation, I do not see any message. This could be confusing (as the user might think that no check has been performed). Would it be possible to have a message stating something like: "No rule violations found. Click to dismiss.".

Tomas-M commented 2 years ago

Ah, I was so busy checking the rules that I forgot the case when all of them pass :) Implemented by 3f83298feb453009b50c0113e4370dc7e7a5dcb9

pasetti commented 2 years ago

The syntactical check appears to be working well.