search

modelica / fmi-standard

Specification of the Functional Mock-up Interface (FMI)
https://fmi-standard.org/
Other
274 stars 85 forks source link

Final review of FMI2.0.1 RC #623

Closed chrbertsch closed 5 years ago

chrbertsch commented 5 years ago

Please inspect the PDF documents of the RC (see below) and comment in case of errors or possible improvements - ASAP but before Oct 1st. In case we do not find blockers, we plan to vote on the release in the FMI steering committee on Oct 2nd.

//Updated files due to misssing pages FunctionalMockupInterface_v2.0.1_RC1.pdf FunctionalMockupInterface_v2.0.1_RC1_with_changemarks.pdf

// Update 2019-09-25 10:43CEST FunctionalMockupInterface_v2.0.1_RC2.pdf FunctionalMockupInterface_v2.0.1_RC2_with_changemarks.pdf FunctionalMockupInterface_v2.0.1-Diff_RC1_RC2.pdf

// Update 2019-09-27 21:22CEST : RC3 added FunctionalMockupInterface_v2.0.1_RC3.pdf FunctionalMockupInterface_v2.0.1_RC3_with_changemarks.pdf FunctionalMockupInterface_v2.0.1-Diff_RC2_RC3.pdf

// Update 2019-10-01 18:41CEST : RC4 added FunctionalMockupInterface_v2.0.1_RC4.pdf FunctionalMockupInterface_v2.0.1_RC4_with_changemarks.pdf FunctionalMockupInterface_v2.0.1-Diff_RC3_RC4.pdf

// Update 2019-10-02 12:24 CEST : RC5 added FunctionalMockupInterface_v2.0.1_RC5.pdf FunctionalMockupInterface_v2.0.1_RC5_with_changemarks.pdf FunctionalMockupInterface_v2.0.1-Diff_RC4_RC5.pdf

// Update 2019-10-06 15:20 CEST: RC6 added (figure numbers fixed) FunctionalMockupInterface_v2.0.1_RC6.pdf FunctionalMockupInterface_v2.0.1_RC6_with_changemarks.pdf FunctionalMockupInterface_v2.0.1-Diff_RC5_RC6.pdf

Please comment on the latest documents

nickbattle commented 5 years ago

These two documents only go to page 72 or 74 (of 129/137)?

chrbertsch commented 5 years ago

These two documents only go to page 72 or 74 (of 129/137)? @nickbattle : Thanks for the hint, I have update the PDFs, now hopefully the all pages are there.

nickbattle commented 5 years ago

Thanks. Both are complete now.

andreas-junghanns commented 5 years ago

[pages refer to the pages in the document WITH change marks]

andreas-junghanns commented 5 years ago
nickbattle commented 5 years ago

The example XML on p100 (of the changemarked version) has commas after the valueReference attribute of the x[1] and x[2] ScalarVariables. This isn't valid XML:

<ScalarVariable name="x[1]" valueReference="0", initial="exact"> <Real/>
</ScalarVariable> <!—index="5" -->
<ScalarVariable name="x[2]" valueReference="1", initial="exact"> <Real/>
</ScalarVariable> <!—index="6" -->

chrbertsch: fixed in RC2 Similarly, the abbreviated examples on p66/67 imply wrongly that there are commas between attributes:

<ScalarVariable name="p" , ...> ... </ScalarVariable>

chrbertsch: fixed in RC2 And I think strictly, the XML encoding attribute on p99 should have a hyphen, "UTF-8" (ie. fmuCheck rejects "UTF8" as an unknown encoding). chrbertsch: fixed in RC2: replaced all occurences of UTF8 by UTF-8

nickbattle commented 5 years ago

Also in the example XML on p99, some of the comments have had the hyphens turned into m-dashes, so a copy from the document fails to parse:

<!—index="1" -->  should be <!--index="1" -->

comment by chrbertsch: replaced 23 occurences of this for RC2

Also the example has some SV and unit errors, as shown by fmuCheck and our own VDMCheck tool:

$ runFmuCheck.sh example.xml
[INFO][FMILIB] XML specifies FMI standard version 2.0
[INFO][FMI2XML] [Line:74] Detected during parsing:
[ERROR][FMI2XML] Start attribute is required for this causality, variability and initial combination
[INFO][FMI2XML] [Line:77] Detected during parsing:
[ERROR][FMI2XML] Start attribute is required for this causality, variability and initial combination
$ VDMCheck.sh example.xml                                                                                                                                    
2.2.7 Variable "x[1]" initial/variability/start <exact>/<continuous>/nil invalid at line 77
2.2.7 ScalarVariable "x[1]" invalid at line 77
2.2.7 Variable "x[2]" initial/variability/start <exact>/<continuous>/nil invalid at line 80
2.2.7 ScalarVariable "x[2]" invalid at line 80
2.2.1 ScalarVariables invalid
2.2.3 SimpleType "Modelica.SIunits.Torque", Real unit "N.m" not defined in UnitDefinitions at line 29
2.2.7 ScalarVariable "torque.tau", Real unit "N.m" not defined in UnitDefinitions at line 57
Errors found.

chrbertsch: added in RC2

   <Unit name="N.m">
      <BaseUnit kg="1" m="2" s="-2"/>
    </Unit>

and 

<Real start="0"/>

for x[1] and x[2]

nickbattle commented 5 years ago

The XML other example on p121 only has the "UTF-8" encoding and N.m units problems:

2.2.3 SimpleType "Modelica.SIunits.Torque", Real unit "N.m" not defined in UnitDefinitions at line 29
2.2.7 ScalarVariable "torque.tau", Real unit "N.m" not defined in UnitDefinitions at line 50

chrbertsch: fixed in RC2

nickbattle commented 5 years ago

Small point, but I note that "?" is included as both a Q-char and an escape in the grammar on p69. Otherwise the sets are disjoint. Is that correct? comment by chrbertsch: see comment below

CThuleHansen commented 5 years ago

On p. 45 regarding VendorAnnotations with type fmi2Annotation it is stated Attribute “name” must be unique with respect to all other elements of the VendorAnnotation list. But this is not the case on p. 57 for Annotations with type fmi2Annotation.

This is confusing as the types are the same, fmi2Annotation but the invariants are different.

chrbertsch: see comments below, opened separate ticket #629

CThuleHansen commented 5 years ago

Unnecessary restriction related to fmi2Discard: Figure 12 on p. 107 shows that fmi2Discard leads to stepFailed. In order to continue simulation from this point, it is necessary to fmi2reset of fmi2SetFMUState according to table on p. 109. However, it is stated on p. 105: fmi2Discard – if the slave computed successfully only a subinterval of the communication

Why is the computed subinterval not valid - in other words, why does it require a reset or setstate in order to continue? Perhaps the restriction can be lifted and the computed subinterval made valid?

chrbertsch: moved discussion to separate issue #630

jph-tavella commented 5 years ago

Some small issues in the changemarks of the text after reading the FunctionalMockupInterface_v2.0.1_RC1_with_changemarks.pdf file.

page 2: rewrite relase in release chrbertsch: fixed in RC2 page 14: rewrite #indclude in #include chrbertsch: fixed in RC2 page 19: rewrite I n in In : chrbertsch: alreday fixed in RC2, see comment from Andreas above page 31: rewrite visibleaccessible in visible accessible: chrbertsch: changed to visible and accessible. Is this OK?: update 27.09.2019: will switch back to visible/accessible as in FMI 2.0

in addition, section 0 are visible at different place in the text; should be an error? chrbertsch: fixed this about 10 times, seems to be an MS Word bug.

nickbattle commented 5 years ago

On p21, the fmi2SetDebugLogging function indicates what happens for nCategories > 0, but not when it equals zero. The fmusdk has interpreted this to mean enable or disable all categories. Can that be clarified?

Also, it is not clear whether this is intended to allow toggling of categories (multiple calls to enable/disable category sets) or whether it is a one-shot call. The fmusdk interprets it as one shot, resetting all categories before setting those passed.

chrbertsch: see comment below, moved to #628

This section ends with "See section 0". chrbertsch: section 0 is fixed in RC 2

chrbertsch commented 5 years ago

Also the example has some SV and unit errors, as shown by fmuCheck and our own VDMCheck tool:

form comment above @nickbattle : could you or someone else please come up with corrections for this?

chrbertsch: fixed in RC2

chrbertsch commented 5 years ago

Small point, but I note that "?" is included as both a Q-char and an escape in the grammar on p69. Otherwise the sets are disjoint. Is that correct?

@nickbattle or someone else: I am no expert on this and cannot decide this. Could you please come up with a proposal?

chrbertsch: please discuss this in #612

nickbattle commented 5 years ago

@chrbertsch The files attached now pass VDMCheck and fmuCheck:

example_3.3.2.xml.txt example_4.3.2.xml.txt

chrbertsch: fixed in RC2

nickbattle commented 5 years ago

Small point, but I note that "?" is included as both a Q-char and an escape in the grammar on p69. Otherwise the sets are disjoint. Is that correct?

@nickbattle or someone else: I am no expert on this and cannot decide this. Could you please come up with a proposal?

I think this inconsistency is present in the Modelica 3.2 grammar as well, so there is perhaps a case for leaving it as it is. If we want to clear it up, I suggest that "?" is a Q-char rather than an escape, since the character would not normally require escaping in most other contexts. chrbertsch: see comment below, discuss this in #612

chrbertsch commented 5 years ago

Small point, but I note that "?" is included as both a Q-char and an escape in the grammar on p69. Otherwise the sets are disjoint. Is that correct?

@nickbattle or someone else: I am no expert on this and cannot decide this. Could you please come up with a proposal?

I think this inconsistency is present in the Modelica 3.2 grammar as well, so there is perhaps a case for leaving it as it is. If we want to clear it up, I suggest that "?" is a Q-char rather than an escape, since the character would not normally require escaping in most other contexts.

Ah, there is already a ticket for this, #612 --> Please discuss there. If this is not solved unexpectedly soon, this will not be part of FMI 2.0.1

chrbertsch commented 5 years ago

On p. 45 regarding VendorAnnotations with type fmi2Annotation it is stated Attribute “name” must be unique with respect to all other elements of the VendorAnnotation list. But this is not the case on p. 57 for Annotations with type fmi2Annotation.

This is confusing as the types are the same, fmi2Annotation but the invariants are different.

URGENT: @ all : I do not know what to do with this. Please comment and come up with a proposal; otherwise this will not be be included in FMI 2.0.1

chrbertsch: opened a separate ticket for this #629

nickbattle commented 5 years ago

On p. 45 regarding VendorAnnotations with type fmi2Annotation it is stated Attribute “name” must be unique with respect to all other elements of the VendorAnnotation list. But this is not the case on p. 57 for Annotations with type fmi2Annotation. This is confusing as the types are the same, fmi2Annotation but the invariants are different.

URGENT: @ALL : I do not know what to do with this. Please comment and come up with a proposal; otherwise this will not be be included in FMI 2.0.1

VDMCheck verifies this, assuming the tool names should be unique in both places. There are no occurrences of a duplicate name in the whole of the FMI Cross Check repository (792 FMUs), so it might be safe to make that the rule?

chrbertsch: opened a separate ticket for this #629

chrbertsch commented 5 years ago

Unnecessary restriction related to fmi2Discard: Figure 12 on p. 107 shows that fmi2Discard leads to stepFailed. In order to continue simulation from this point, it is necessary to fmi2reset of fmi2SetFMUState according to table on p. 109. However, it is stated on p. 105: fmi2Discard – if the slave computed successfully only a subinterval of the communication

Why is the computed subinterval not valid - in other words, why does it require a reset or setstate in order to continue? Perhaps the restriction can be lifted and the computed subinterval made valid?

URGENT: Could someone familiar with this please comment? Shall we open a separate issue to discuss this? In case of no fast solution/decision , this will not be included in FMI 2.0.1

chrbertsch: moved discussion to separate issue #630

chrbertsch commented 5 years ago

On p21, the fmi2SetDebugLogging function indicates what happens for nCategories > 0, but not when it equals zero. The fmusdk has interpreted this to mean enable or disable all categories. Can that be clarified?

Also, it is not clear whether this is intended to allow toggling of categories (multiple calls to enable/disable category sets) or whether it is a one-shot call. The fmusdk interprets it as one shot, resetting all categories before setting those passed.

@ all : Please comment and come up with a proposal for a wording. Possibly open a new ticket for this. Otherwise this will not be included in FMI 2.0.1

Changed ticket: move to separate issue #628

nickbattle commented 5 years ago

URGENT: @ all : Please comment and come up with a proposal for [setDebugLogging] wording. Possibly open a new ticket for this. Otherwise this will not be included in FMI 2.0.1

@chrbertsch I'm not confident enough to state the intended semantics. But it's hardly critical, so I'll move this out to a new issue.

Commented by chrbertsch: move to separate issue #628

chrbertsch commented 5 years ago

Clarification added wr.t. to fmiversion string in XML remaining to be "2.0" also for FMI 2.0.1 FMUs added to RC2:

image

image

I also updated the docuemtation string in https://github.com/modelica/fmi-design/blob/master/FMI_2.0.1/fmi2ModelDescription.xsd image

Does this make sense? could someone please comment?

chrbertsch: as there was no objection, I assume acceptance

andreas-junghanns commented 5 years ago

To leave the version string at "2.0" was what we discussed. I remember the argument that 2.0 importers should be able to read 2.0.1 FMUs and recognise 2.0.1 FMUs as 2.0. Correct?

chrbertsch commented 5 years ago

To leave the version string at "2.0" was what we discussed. I remember the argument that 2.0 importers should be able to read 2.0.1 FMUs and recognise 2.0.1 FMUs as 2.0. Correct?

Yes. Could someone please comment on my proposed wording above? Does it make sense to modify the documentation string on the xsd as proposed? Wouldn't change functionality comapred to FMI 2.0, but then the xsd-files are not bit identical. But if we do not change it, we have something wrong there.

chrbertsch: done on the v2.0.1 branch

chrbertsch commented 5 years ago

I uploaded RC2, a comparison with FM2.0 with changemarks and a Diff between RC1 and RC2 --> see main ticket text on top of this page @nickbattle , @andreas-junghanns , @andreas-junghanns , @jph-tavella @CThuleHansen : Thanks for reviewing an commenting I have included all findings from comments above (if I understood them and if they have not raised new issues) and commented on them. There are some questions from my side which are in bold - please comment on my questions.

I edited also the frontpage to adapt it to 2.0.1

Some comments raised totally new topics- I have or will move them to separate issues. As it is very late in the process - we plan to vote on the release of FMI 2.0.1 in one weeks time - if no one comes up immediately with a contrete proposal for the wording that gets approved by the design group, we will not include these changes in FMI 2.0.1

@ all: Please check RC2 and comment from now on RC2 and mention in your ticket, which PDF you are referring to. Thanks!

andreas-junghanns commented 5 years ago

Comment(s) page numbers from the RC1-RC2 compare document:

chrbertsch: fixed in RC3

andreas-junghanns commented 5 years ago

RC 1:

chrbertsch commented 5 years ago

RC 1:

* at the end of the table of content, there is an stray "1." below Appendix B _chrbertsch: don't see this in RC2, please check_

* the release date at the top of the page is July 12, 2019; _chrbertsch: don't see this in RC2, please check_

* contributors list for 2.0.1 is missing  _chrbertsch: please use #633 to collect contributors_

* page 49, there is a lot of space above the last paragraph of the page (and table)  _chrbertsch: don't see this in RC2, please check; will check pagebreaks and empty lines for RC3_

* page 1 and page 2, release date of 2.0.1 is still July 12, 2019 (won´t it be October 2nd?)  chrbertsch: don't see this in RC2, please check_

@modelica/fmi-design: please comment only on the latest RC, currently RC2. Thanks!

beutlich commented 5 years ago

page 68:

[Note: especially section 4.4.17 states that backslashes "\" are forbidden as path separator only forward slashes "/" are allowed.]

Should there be a comma or "and" before "only"?

chrbertsch: will be fixed in RC3

beutlich commented 5 years ago

Just as a reminder, since not available in the RC2.

  1. Would be good if the final release again contains the intended PDF meta data
    • Title: Functional Mock-up Interface for Model Exchange and Co-Simulation, Version 2.0.1
    • Author: Modelica Association Project "FMI"

chrbertsch: Will be fixed in RC3

  1. Also, paper format should be changed from letter (RC2) back to A4 (as v2.0).

chrbertsch: Will be fixed in RC3

  1. Last, not least, the PDF version was increased from 1.6 (as v2.0) to 1.7 (RC2). Not sure, if this really is necessary.

chrbertsch: starting with RC3 we will use the Word 2016 functionality for exporting the pdf instead of the Adope PDF printer. The new genereated PDF files are PDF version 1.6

andreas-junghanns commented 5 years ago

RC 1:

* at the end of the table of content, there is an stray "1." below Appendix B _chrbertsch: don't see this in RC2, please check_

* the release date at the top of the page is July 12, 2019; _chrbertsch: don't see this in RC2, please check_

* contributors list for 2.0.1 is missing  _chrbertsch: please use #633 to collect contributors_

* page 49, there is a lot of space above the last paragraph of the page (and table)  _chrbertsch: don't see this in RC2, please check; will check pagebreaks and empty lines for RC3_

* page 1 and page 2, release date of 2.0.1 is still July 12, 2019 (won´t it be October 2nd?)  chrbertsch: don't see this in RC2, please check_

@modelica/fmi-design: please comment only on the latest RC, currently RC2. Thanks!

Sorry, I checked, none of the issues reported are in RC2.

beutlich commented 5 years ago

to

The #include directives with quotes ("") should be used for header-files distributed in the FMU instead of using angle brackets (<>) .

chrbertsch: Fixed in RC3

The sub-directory licenses can be used to bundle all license files

Looks like this was copied from adoc. Change licenses to "licenses" in fixed font, similar as "sources" or "documentation" on the same page. chrbertsch: Fixed in RC3

beutlich commented 5 years ago

page 5: English grammar issue. Change

a set of test FMUs are provided

to

a set of test FMUs is provided

chrbertsch: Fixed in RC3

beutlich commented 5 years ago

Another PDF issue: The page links from the table of contents (pages 6 and 7) are no longer clickable in the RC2 (but were in v2.0).

chrbertsch: fixed in RC3 (by the new PDF creation method "export PDF" from Word 2016

beutlich commented 5 years ago

// Update 2019-09-25 10:43CEST FunctionalMockupInterface_v2.0.1_RC2.pdf FunctionalMockupInterface_v2.0.1_RC2_with_changemarks.pdf

I can see changes in FunctionalMockupInterface_v2.0.1_RC2.pdf that are not present/marked in FunctionalMockupInterface_v2.0.1_RC2_with_changemarks.pdf.

chrbertsch : see comment https://github.com/modelica/fmi-standard/issues/623#issuecomment-536188036

andreas-junghanns commented 5 years ago

RC2 (comments mostly pages 100 to 126 in reverse because I am reading backwards, sorry):

General comment: finding key words starting with fmi2 that are not fixed-size font should be "easy" in the source - I think I should stop spamming you here and you try to find those systematically. OK? chrbertsch: done. changed about 50 occurences to fixed-size."

beutlich commented 5 years ago

page 5: The one link on that page links to https://fmi-standard.org/downloads):, which cannot be resolved. In v2.0 it correctly links to https://fmi-standard.org/downloads. chrbertsch: in the RC3 pdf the link works works

beutlich commented 5 years ago

page 5: The "Library to test connected FMUs" is not available from https://fmi-standard.org/downloads. Not sure, if it shall be added on the web page or if that tool should be removed from the spec. chrbertsch: yes it is available on the Website under downloads, see Test FMUs.

chrbertsch commented 5 years ago

I can see changes in FunctionalMockupInterface_v2.0.1_RC2.pdf that are not present/marked in FunctionalMockupInterface_v2.0.1_RC2_with_changemarks.pdf.

We use the comparison mechanism described in https://github.com/modelica/fmi-standard/blob/support/v2.0.x/CONTRIBUTING.adoc to create the document with changemarks. @beutlich : Are the other changes important? Then please provide an example.

chrbertsch commented 5 years ago

from @andreas-junghanns comment above:

  • page 105 of RC2: Description cell of fmi2Terminated: true -> should be fixed-size font (should it be fmi2True?) chrbertsch: done in RC3

Now reads: Returns fmi2True, if the slave wants to terminate the simulation. Can be called after fmi2DoStep(..) returned fmi2Discard. Use fmi2LastSuccessfulTime to determine the time instant at which the slave terminated.

Is this correct?

chrbertsch commented 5 years ago

@andreas-junghanns : from your comment above on RC2

same page (104) above: "fmi2OK – if the communication step was computed successfully until its end." -> should end with ',' as the next line continues the sentense

Do not understand. This is part of a list of option for "The function returns:" where every sentence ends up with a '.'

andreas-junghanns commented 5 years ago

@andreas-junghanns : from your comment above on RC2

same page (104) above: "fmi2OK – if the communication step was computed successfully until its end." -> should end with ',' as the next line continues the sentense

Do not understand. This is part of a list of option for "The function returns:" where every sentence ends up with a '.'

Mmh, I thought this entire list is one sentence. But I might be wrong.

beutlich commented 5 years ago

@beutlich : Are the other changes important? Then please provide an example.

I do not know. I just observed it and wondered.

chrbertsch commented 5 years ago

I uploaded RC3, a comparison with FM2.0 release with changemarks and a Diff between RC2 and RC3 --> see main ticket text on top of this page

I have included all findings from comments above that have been clarified and commented on them. There are very few small questions open now.

Some comments raised totally new topics- I have or will move them to separate issues. As it is very late in the process - we plan to vote on the release of FMI 2.0.1 on Wednesday - if no one comes up immediately with a concrete proposal for the wording that gets approved by the design group, we will not include these changes in FMI 2.0.1 Open are: #630, #629

It would help, if more people go through the tickets list #616 of fixed tickets and check if they are really included correctly in RC3 and if the wording is fine in #616 for the changelog.

@ all: Please check RC3 and comment from now on RC3 and mention in your ticket, which PDF you are referring to. Thanks!

beutlich commented 5 years ago

page 5: The "Library to test connected FMUs" is not available from https://fmi-standard.org/downloads. Not sure, if it shall be added on the web page or if that tool should be removed from the spec. chrbertsch: yes it is available on the Website under downloads, see Test FMUs.

Copied to https://github.com/modelica/FMIModelicaTest. You might want to update the web site.

nickbattle commented 5 years ago

The RC PDFs don't have a table of contents that a PDF viewer can understand, whereas the original 2.0 PDF did. This is useful for quickly skipping to a particular section in the document. Is that an option when generating the PDF?

(I'm using Okular on Linux. I can't try this with other PDF viewers to verify)

chrbertsch: fixed in principle, see below

clagms commented 5 years ago

The RC PDFs don't have a table of contents that a PDF viewer can understand, whereas the original 2.0 PDF did. This is useful for quickly skipping to a particular section in the document. Is that an option when generating the PDF?

I can confirm this on Windows: no bookmarks available.

chrbertsch: fixed in principle, see below

chrbertsch commented 5 years ago

In the RC2.pdf the table of centents was there. @clagms andn @nickbattle : Could you please confirm? in RC3 we switched from Adobe PDF for generating the PDF to Word 2016 -->Export --> credate PDF/XPS Docuemnent. There I do not find options to create the bookmarks image Does anyone have hints how one coould do it? Otherwise I suggest to switch back to Adope PDF printer to generate the PDF

chrbertsch: fixed in principle, see below