Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-34005

sal may use an unsuitable type for an XML enumeration

    XMLWordPrintable

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: None
    • Labels:
    • Story Points:
      0.5
    • Sprint:
      TSSW Sprint - Mar 28 - Apr 11
    • Team:
      Telescope and Site
    • Urgent?:
      No

      Description

      The ATWhiteLight XML has two very large mask words that I have written enum constants for. The masks come from a low-level controller (I don't control them). Here is one:

       <Enumeration>
          ChillerL22Alarms_EXTERNAL_RTD_SENSOR_OPEN = 0x1,
          ChillerL22Alarms_EXTERNAL_RTD_SENSOR_SHORT = 0x2,
          ...
          ChillerL22Alarms_FRONT_LEFT_FAN_OPEN = 0x40000000,
          ChillerL22Alarms_FRONT_RIGHT_FAN_OPEN = 0x80000000
        </Enumeration>
      

      The problem is that sal declares these as constants of type long, which is not large enough to hold the final value. We need unsigned long here.

      One possible solution is to allow the user to specify a data type as an argument in the Enumeration tag. That is maximally flexible and avoids any "magic". It also is a very important signal to the user that the type of any field that uses this enumeration must not be "int" or "long". Thus I think it is a far better solution that simply picking a suitable type and using it.

      It'd be neat if the enumeration could get its type from all topic fields that are of the enumeration type (and they would all have to match), but that seems likely to be more work than it is worth.

      My short-term workaround is to omit the final value, though I'm a bit disappointed to have to do that. This brings up another request:

      Please also consider supporting some way to include comment lines in enumeration values. That way I could easily document that I'm leaving out a value (and why). It would also be useful for documenting "this mask value not used" (of which the AT white light chiller has a surprising number). I tried three things, none of which worked: a standard XML comment (which I think is the best solution, since this is an XML file and editors will display it as comment), a line beginning with //, and a line beginning with #. It's not a big deal and is not worth a lot of work. For this case I will just change the too-large value to a comment appearing right after the close enumeration tag.

        Attachments

          Issue Links

            Activity

            Hide
            dmills Dave Mills added a comment - - edited

            If it begins a value with 0x then it is a mask and thus unsigned ?
            It is a rule/convention/sal-constraint in that case and gets documented accordingly.

            Show
            dmills Dave Mills added a comment - - edited If it begins a value with 0x then it is a mask and thus unsigned ? It is a rule/convention/sal-constraint in that case and gets documented accordingly.
            Hide
            rowen Russell Owen added a comment - - edited

            I would be really uncomfortable with that kind of magic. I think we have masks that use integers (though I hope not many).

            I'd rather just close this as "won't fix" rather than use an overly simplified heuristic to pick a type.

            A principled approach would be to examine the range of values and identify a suitable data type. But that sounds like a lot of work and difficult to test.

            I strongly suggest either closing this as won't fix or letting the user specify a type (with a suitable default).

            Show
            rowen Russell Owen added a comment - - edited I would be really uncomfortable with that kind of magic. I think we have masks that use integers (though I hope not many). I'd rather just close this as "won't fix" rather than use an overly simplified heuristic to pick a type. A principled approach would be to examine the range of values and identify a suitable data type. But that sounds like a lot of work and difficult to test. I strongly suggest either closing this as won't fix or letting the user specify a type (with a suitable default).
            Hide
            dmills Dave Mills added a comment -

            Addressed as part of PR#189

            Show
            dmills Dave Mills added a comment - Addressed as part of PR#189
            Hide
            dmills Dave Mills added a comment - - edited

            I added a type=short to one of the Test Enumerations
            <Enumeration type="short">Int0Enum_One, Int0Enum_Two, Int0Enum_Three</Enumeration>

            and got these const in the idl

            const long indexEnumeration_any=1;
            long ack; // @Metadata=(Units="unitless",Description="Final ack code, typically a SAL__CMD constant.")
            const long arrays_Int0ValueEnum_Zero=0;
            const long arrays_Int0ValueEnum_Two=2;
            const long arrays_Int0ValueEnum_Four=04;
            const long arrays_Int0ValueEnum_Five=0x05;
            const short scalars_Int0Enum_One=1;
            const short scalars_Int0Enum_Two=2;
            const short scalars_Int0Enum_Three=3;
            const long Test_shared_Enum_One=1;
            const long Test_shared_Enum_Two=2;
            const long Test_shared_Enum_Three=3;
            const long Test_shared_ValueEnum_Zero=0;
            const long Test_shared_ValueEnum_Two=2;
            const long Test_shared_ValueEnum_Four=04;
            const long Test_shared_ValueEnum_Five=0x05;

            Show
            dmills Dave Mills added a comment - - edited I added a type=short to one of the Test Enumerations <Enumeration type="short">Int0Enum_One, Int0Enum_Two, Int0Enum_Three</Enumeration> and got these const in the idl const long indexEnumeration_any=1; long ack; // @Metadata=(Units="unitless",Description="Final ack code, typically a SAL__CMD constant.") const long arrays_Int0ValueEnum_Zero=0; const long arrays_Int0ValueEnum_Two=2; const long arrays_Int0ValueEnum_Four=04; const long arrays_Int0ValueEnum_Five=0x05; const short scalars_Int0Enum_One=1; const short scalars_Int0Enum_Two=2; const short scalars_Int0Enum_Three=3; const long Test_shared_Enum_One=1; const long Test_shared_Enum_Two=2; const long Test_shared_Enum_Three=3; const long Test_shared_ValueEnum_Zero=0; const long Test_shared_ValueEnum_Two=2; const long Test_shared_ValueEnum_Four=04; const long Test_shared_ValueEnum_Five=0x05;
            Hide
            rbovill Rob Bovill added a comment -

            PR reviewed in GitHub.

            Show
            rbovill Rob Bovill added a comment - PR reviewed in GitHub.

              People

              Assignee:
              dmills Dave Mills
              Reporter:
              rowen Russell Owen
              Reviewers:
              Rob Bovill
              Watchers:
              Dave Mills, Rob Bovill, Russell Owen
              Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  Jenkins

                  No builds found.