You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@plc4x.apache.org by Łukasz Dywicki <lu...@code-house.org> on 2020/08/25 23:22:25 UTC

[DISCUSS] Possible changes to discriminator field handling

I been trying to write new mspecs and I found that discriminator
handling is quite limiting in current form.

For example it is possible to declare a type argument, but it is not
possible to use it as a divider.
It is also not possible to use a virtual or manual field as a
discriminator which could very well be source for type information. I am
aware that virtual field is computed at runtime with given expression.
I am not entirely sure of all implications for changing the handling.
Currently discriminator is a simple field which gets registered in
discriminator set.

What do you think about making a `discriminator` rather an modifier
which could be placed for type arguments and also in other places? At
the moment any use of [typeSwitch] with undeclared discriminator causes
null pointers in freemarker templates and failure.

Cheers,
Łukasz

Re: [DISCUSS] Possible changes to discriminator field handling

Posted by Christofer Dutz <ch...@c-ware.de>.
I don't really think so ... 

I also think there are mspec discriminated types without any discriminator field ... 
If this doesn't work, that's a bug that should be fixed.

And I just found an example ... in the S7 mspec the type of parameter also decides the type of payload (1-to-1 match)

[discriminatedType 'S7Payload' [uint 8 'messageType', S7Parameter 'parameter']
    [typeSwitch 'parameter.parameterType', 'messageType'
        ['0x04','0x03' S7PayloadReadVarResponse [S7Parameter 'parameter']
            [array S7VarPayloadDataItem 'items' count 'CAST(parameter, S7ParameterReadVarResponse).numItems' ['lastItem']]
        ]
        ['0x05','0x01' S7PayloadWriteVarRequest [S7Parameter 'parameter']
            [array S7VarPayloadDataItem 'items' count 'COUNT(CAST(parameter, S7ParameterWriteVarRequest).items)' ['lastItem']]
        ]
        ['0x05','0x03' S7PayloadWriteVarResponse [S7Parameter 'parameter']
            [array S7VarPayloadStatusItem 'items' count 'CAST(parameter, S7ParameterWriteVarResponse).numItems']
        ]
        ['0x00','0x07' S7PayloadUserData [S7Parameter 'parameter']
            [array S7PayloadUserDataItem 'items' count 'COUNT(CAST(parameter, S7ParameterUserData).items)' ['CAST(CAST(parameter, S7ParameterUserData).items[0], S7ParameterUserDataItemCPUFunctions).cpuFunctionType']]
        ]
    ]
]

This doesn't have a single discriminator field and works nicely.

Chris





Am 26.08.20, 16:23 schrieb "Łukasz Dywicki" <lu...@code-house.org>:

    Yes it works cause the error and function bits are declared as
    discriminators. Try making response bit first in type switch. Then its
    gonna fail. This is because you need to explicitly declare a first
    discriminator field.
    It is not a big deal, however when we have header and payload parts one
    is rather constant and other is rather dynamic and in case of CAN some
    information comes from header and influence how parsing of payload
    should be made.

    See below:

    [type 'SocketCANFrame'
        [simple int 32 'rawId']
        [virtual int 32 'identifier'
            'STATIC_CALL("..", rawId)'
        ]
        [virtual bit 'extended'
            'STATIC_CALL("..", rawId)'
        ]
        [virtual bit 'remote'
            'STATIC_CALL("..", rawId)'
        ]
        [virtual bit 'error'
            'STATIC_CALL("..", rawId)'
        ]
        [implicit uint 8 'size' 'COUNT(data)']
        [CANOpenPDU 'data' ['remote', 'extended']]
        [typeSwitch 'extended'
            ['true' ExtendedFrame
               // flags
            ]
            ['false' StandardFrame
               // padding
            ]
        ]
    ]

    [type CANOpenPDU [bit 'remote', bit 'extended']
      [typeSwitch 'remote', 'extended'
         // this wont generate
         ['true', 'true'  ExtendedRemotePDU
         ]
      ]
    ]

    Best,
    Łukasz


    On 26.08.2020 10:32, Christofer Dutz wrote:
    > Here the part in the modbus.mspec I was talking about:
    > 
    > [discriminatedType 'ModbusPDU' [bit 'response']
    >     [discriminator bit         'error']
    >     [discriminator uint 7      'function']
    >     [typeSwitch 'error','function','response'
    >         ['true'                     ModbusPDUError
    >             [simple     uint 8      'exceptionCode']
    >         ]
    > 
    >         // Bit Access
    >         ['false','0x02','false'     ModbusPDUReadDiscreteInputsRequest
    >             [simple     uint 16     'startingAddress']
    >             [simple     uint 16     'quantity']
    >         ]
    >         ['false','0x02','true'      ModbusPDUReadDiscreteInputsResponse
    >             [implicit   uint 8      'byteCount'     'COUNT(value)']
    >             [array      int 8       'value'         count   'byteCount']
    >         ]
    > 
    > So the input values in the "typeSwitch" are actually expressions that are evaluated. It doesn't really care about if something's a discriminator.
    > A discriminator is only interesting when serializing as in this case it doesn't store the value in the pojo but gets the value from the type instead.
    > 
    > Chris
    > 
    > 
    > 
    > Am 26.08.20, 10:29 schrieb "Christofer Dutz" <ch...@c-ware.de>:
    > 
    >     Hi Lukasz,
    > 
    >     It would be pretty simple to extend the code generation to make the virtual fields available in the parser ... 
    >     I just didn't do it as I then didn't see the need. Should be extremely simple to implement. (Sort of just a copy+paste of the pojo template)
    > 
    >     You can use type-arguments in typeSwitches ... just have a look at the modbus mspec.
    > 
    >     What do you mean with "undeclared discriminators"?
    > 
    >     Chris
    > 
    > 
    > 
    >     Am 26.08.20, 01:22 schrieb "Łukasz Dywicki" <lu...@code-house.org>:
    > 
    >         I been trying to write new mspecs and I found that discriminator
    >         handling is quite limiting in current form.
    > 
    >         For example it is possible to declare a type argument, but it is not
    >         possible to use it as a divider.
    >         It is also not possible to use a virtual or manual field as a
    >         discriminator which could very well be source for type information. I am
    >         aware that virtual field is computed at runtime with given expression.
    >         I am not entirely sure of all implications for changing the handling.
    >         Currently discriminator is a simple field which gets registered in
    >         discriminator set.
    > 
    >         What do you think about making a `discriminator` rather an modifier
    >         which could be placed for type arguments and also in other places? At
    >         the moment any use of [typeSwitch] with undeclared discriminator causes
    >         null pointers in freemarker templates and failure.
    > 
    >         Cheers,
    >         Łukasz
    > 
    > 


Re: [DISCUSS] Possible changes to discriminator field handling

Posted by Łukasz Dywicki <lu...@code-house.org>.
Yes it works cause the error and function bits are declared as
discriminators. Try making response bit first in type switch. Then its
gonna fail. This is because you need to explicitly declare a first
discriminator field.
It is not a big deal, however when we have header and payload parts one
is rather constant and other is rather dynamic and in case of CAN some
information comes from header and influence how parsing of payload
should be made.

See below:

[type 'SocketCANFrame'
    [simple int 32 'rawId']
    [virtual int 32 'identifier'
        'STATIC_CALL("..", rawId)'
    ]
    [virtual bit 'extended'
        'STATIC_CALL("..", rawId)'
    ]
    [virtual bit 'remote'
        'STATIC_CALL("..", rawId)'
    ]
    [virtual bit 'error'
        'STATIC_CALL("..", rawId)'
    ]
    [implicit uint 8 'size' 'COUNT(data)']
    [CANOpenPDU 'data' ['remote', 'extended']]
    [typeSwitch 'extended'
        ['true' ExtendedFrame
           // flags
        ]
        ['false' StandardFrame
           // padding
        ]
    ]
]

[type CANOpenPDU [bit 'remote', bit 'extended']
  [typeSwitch 'remote', 'extended'
     // this wont generate
     ['true', 'true'  ExtendedRemotePDU
     ]
  ]
]

Best,
Łukasz


On 26.08.2020 10:32, Christofer Dutz wrote:
> Here the part in the modbus.mspec I was talking about:
> 
> [discriminatedType 'ModbusPDU' [bit 'response']
>     [discriminator bit         'error']
>     [discriminator uint 7      'function']
>     [typeSwitch 'error','function','response'
>         ['true'                     ModbusPDUError
>             [simple     uint 8      'exceptionCode']
>         ]
> 
>         // Bit Access
>         ['false','0x02','false'     ModbusPDUReadDiscreteInputsRequest
>             [simple     uint 16     'startingAddress']
>             [simple     uint 16     'quantity']
>         ]
>         ['false','0x02','true'      ModbusPDUReadDiscreteInputsResponse
>             [implicit   uint 8      'byteCount'     'COUNT(value)']
>             [array      int 8       'value'         count   'byteCount']
>         ]
> 
> So the input values in the "typeSwitch" are actually expressions that are evaluated. It doesn't really care about if something's a discriminator.
> A discriminator is only interesting when serializing as in this case it doesn't store the value in the pojo but gets the value from the type instead.
> 
> Chris
> 
> 
> 
> Am 26.08.20, 10:29 schrieb "Christofer Dutz" <ch...@c-ware.de>:
> 
>     Hi Lukasz,
> 
>     It would be pretty simple to extend the code generation to make the virtual fields available in the parser ... 
>     I just didn't do it as I then didn't see the need. Should be extremely simple to implement. (Sort of just a copy+paste of the pojo template)
> 
>     You can use type-arguments in typeSwitches ... just have a look at the modbus mspec.
> 
>     What do you mean with "undeclared discriminators"?
> 
>     Chris
> 
> 
> 
>     Am 26.08.20, 01:22 schrieb "Łukasz Dywicki" <lu...@code-house.org>:
> 
>         I been trying to write new mspecs and I found that discriminator
>         handling is quite limiting in current form.
> 
>         For example it is possible to declare a type argument, but it is not
>         possible to use it as a divider.
>         It is also not possible to use a virtual or manual field as a
>         discriminator which could very well be source for type information. I am
>         aware that virtual field is computed at runtime with given expression.
>         I am not entirely sure of all implications for changing the handling.
>         Currently discriminator is a simple field which gets registered in
>         discriminator set.
> 
>         What do you think about making a `discriminator` rather an modifier
>         which could be placed for type arguments and also in other places? At
>         the moment any use of [typeSwitch] with undeclared discriminator causes
>         null pointers in freemarker templates and failure.
> 
>         Cheers,
>         Łukasz
> 
> 

Re: [DISCUSS] Possible changes to discriminator field handling

Posted by Christofer Dutz <ch...@c-ware.de>.
Here the part in the modbus.mspec I was talking about:

[discriminatedType 'ModbusPDU' [bit 'response']
    [discriminator bit         'error']
    [discriminator uint 7      'function']
    [typeSwitch 'error','function','response'
        ['true'                     ModbusPDUError
            [simple     uint 8      'exceptionCode']
        ]

        // Bit Access
        ['false','0x02','false'     ModbusPDUReadDiscreteInputsRequest
            [simple     uint 16     'startingAddress']
            [simple     uint 16     'quantity']
        ]
        ['false','0x02','true'      ModbusPDUReadDiscreteInputsResponse
            [implicit   uint 8      'byteCount'     'COUNT(value)']
            [array      int 8       'value'         count   'byteCount']
        ]

So the input values in the "typeSwitch" are actually expressions that are evaluated. It doesn't really care about if something's a discriminator.
A discriminator is only interesting when serializing as in this case it doesn't store the value in the pojo but gets the value from the type instead.

Chris



Am 26.08.20, 10:29 schrieb "Christofer Dutz" <ch...@c-ware.de>:

    Hi Lukasz,

    It would be pretty simple to extend the code generation to make the virtual fields available in the parser ... 
    I just didn't do it as I then didn't see the need. Should be extremely simple to implement. (Sort of just a copy+paste of the pojo template)

    You can use type-arguments in typeSwitches ... just have a look at the modbus mspec.

    What do you mean with "undeclared discriminators"?

    Chris



    Am 26.08.20, 01:22 schrieb "Łukasz Dywicki" <lu...@code-house.org>:

        I been trying to write new mspecs and I found that discriminator
        handling is quite limiting in current form.

        For example it is possible to declare a type argument, but it is not
        possible to use it as a divider.
        It is also not possible to use a virtual or manual field as a
        discriminator which could very well be source for type information. I am
        aware that virtual field is computed at runtime with given expression.
        I am not entirely sure of all implications for changing the handling.
        Currently discriminator is a simple field which gets registered in
        discriminator set.

        What do you think about making a `discriminator` rather an modifier
        which could be placed for type arguments and also in other places? At
        the moment any use of [typeSwitch] with undeclared discriminator causes
        null pointers in freemarker templates and failure.

        Cheers,
        Łukasz



Re: [DISCUSS] Possible changes to discriminator field handling

Posted by Christofer Dutz <ch...@c-ware.de>.
Hi Lukasz,

It would be pretty simple to extend the code generation to make the virtual fields available in the parser ... 
I just didn't do it as I then didn't see the need. Should be extremely simple to implement. (Sort of just a copy+paste of the pojo template)

You can use type-arguments in typeSwitches ... just have a look at the modbus mspec.

What do you mean with "undeclared discriminators"?

Chris



Am 26.08.20, 01:22 schrieb "Łukasz Dywicki" <lu...@code-house.org>:

    I been trying to write new mspecs and I found that discriminator
    handling is quite limiting in current form.

    For example it is possible to declare a type argument, but it is not
    possible to use it as a divider.
    It is also not possible to use a virtual or manual field as a
    discriminator which could very well be source for type information. I am
    aware that virtual field is computed at runtime with given expression.
    I am not entirely sure of all implications for changing the handling.
    Currently discriminator is a simple field which gets registered in
    discriminator set.

    What do you think about making a `discriminator` rather an modifier
    which could be placed for type arguments and also in other places? At
    the moment any use of [typeSwitch] with undeclared discriminator causes
    null pointers in freemarker templates and failure.

    Cheers,
    Łukasz


Re: [DISCUSS] Possible changes to discriminator field handling

Posted by Otto Fowler <ot...@gmail.com>.
 Can you give examples?

On August 25, 2020 at 19:22:40, Łukasz Dywicki (luke@code-house.org) wrote:

I been trying to write new mspecs and I found that discriminator
handling is quite limiting in current form.

For example it is possible to declare a type argument, but it is not
possible to use it as a divider.
It is also not possible to use a virtual or manual field as a
discriminator which could very well be source for type information. I am
aware that virtual field is computed at runtime with given expression.
I am not entirely sure of all implications for changing the handling.
Currently discriminator is a simple field which gets registered in
discriminator set.

What do you think about making a `discriminator` rather an modifier
which could be placed for type arguments and also in other places? At
the moment any use of [typeSwitch] with undeclared discriminator causes
null pointers in freemarker templates and failure.

Cheers,
Łukasz