OCPP 1.6 SampledValue does not allow optional fields to be omitted

[robert-s-ubi]

Hi,

the OCPP 1.6 "SampledValue" class only offers constructors which set all optional fields to default values, rather than leaving them null. Setting them to null to omit them is prohibited by the validator method.

This prevents creating a compliant SampledValue for e.g. Power.Factor, which does not have a unit. Also, for Frequency, the specification says that Hz is always assumed, but nonetheless it seems more appropriate to omit this field rather than transmitting "Wh" in it.

Now I'm unsure how to fix this. Would it be possible to just remove all the this.set*() calls (except this.setValue()) from the constructors, or will this break things? The alternative would be to omit the validator call for the set methods in case of a null argument, e.g. "if (unit != null && !this.isValidUnit(unit)) {".

What fix would you suggest?

[TVolden]

Hi @robert-s-ubi,

That sounds like a good solution. It does however introduce a null pointer exception risk (which may be considered a breaking change to the interface). A compromise could be to have the get method return the default value if the field is null.

How does that sound?

[robert-s-ubi]

Hi Thomas,

thank you very much for your quick response. I have created a pull request with a proposed solution, which also cleans up a number of inconsistencies in the class (enum fields were allowed to be set to null, String fields were not).

The basic change in behavior is that the fields are no longer initialized to the default values in the constructors, but rather left at null, so they will be omitted when serializing.

For compatibility, the getter methods instead return the default value in case the stored field is null. With one important exemption: For the "unit" field, the default is "Wh" only for "Energy" measurands, as per the specification. So for measurands which do not start with "Energy", the getUnit() method will now return null instead of "Wh" in case the "unit" field has not been set to non-null, for full compliance with the specification.

[TVolden]

Hi @robert-s-ubi,

I can see the change got merged: #140

Did this solve the issue? If so, can I get you to close it?

[robert-s-ubi]

Ah, I forgot to close the issue. Thanks for the reminder!

[TVolden]

No problem. I really appreciate all your good work.