KELLERAGfuerDruckmesstechnik/KellerAg.Shared.IoT.Converters

IoTConvert.GsmCommunicationJsonToBusinessObject fails with locals not using dot as decimal separator

Closed this issue · 4 comments

alois-maierhofer commented

IoTConvert.GsmCommunicationJsonToBusinessObject expects a json formatted string as parameter.

Problem:
The methods that are called internally are using float.Parse(s) without passing InvariantCulture as NumberFormatInfo parameter so the process fails if executed on a system where the decimal separator is not a ".".
Example this happens in the (internal) CommandRuleCapitalM.ParseCurrentValuesOfSelectedChannels method

Some further thoughts:

  1. The GsmCommunicationToJson(string) method returns numeric values as strings but should use numerics.
    Example:
    "F": {
    "e": "0"
    }
    should be:
    "F": {
    "e": 0
    }

  2. GsmCommunicationToJson(string) should return a JObject instead of a string. Is there a reason why the methods expecting a JObject are not public?

  3. Consequently GsmCommunicationJsonToBusinessObject should take a JObject instead of (or optionally) a string.

As a work around we can set CurrentCulture to "en-US" but would like to get rid of this as soon as possible.

In general this library is of great help to us to process files sent to us from the Keller waterlevel sensors. Thanks for that!

Best wishes
AM

cBashTN commented

I forget to upload the source. Wanted to do this for a long time, but couldn't find the time.

I'm on holidays until KW08 and will upload the source and produce an updated version.

Of course, I will bugfix your found issue/s, too.

alois-maierhofer commented

Hi Sebastian,
any news on this so far?
I could do a PR for this but I understand you have a more current version of the source code that contains the fix already.

BR
AM

cBashTN commented

Sorry for the long wait. This issue was on my todo list but it would not make it to the top.... :-(

I could verify the issue -> 5e7e450#r84188844

Today, I finally uploaded all the missing source. We also fixed the issue with 2801f20
I also updated the nuget package, but in your case you might better just take the project/s to compile 'em yourself.

Next week I plan to upgrade the live editor, too, and hope to add .NET 6 support.

Again, sorry for the long waiting time!

alois-maierhofer commented