Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Go DS SDK encode the float as base64 string by little-endian binary #457

Closed
weichou1229 opened this issue Mar 4, 2020 · 4 comments · Fixed by #458
Closed

Go DS SDK encode the float as base64 string by little-endian binary #457

weichou1229 opened this issue Mar 4, 2020 · 4 comments · Fixed by #458
Assignees
Labels
3-high priority denoting release-blocking issues bug Something isn't working hold Intended for PRs we want to flag for ongoing review ireland
Milestone

Comments

@weichou1229
Copy link
Member

weichou1229 commented Mar 4, 2020

Modify the float base64 string encoding function.
Convert binary from big-endian to little-endian and then encode the float value to base64 string.

Proposal:
Change the ValueToString function

  1. Convert cv.NumericValue to little-endian binary
  2. Encode the binary to base64 string

str = base64.StdEncoding.EncodeToString(cv.NumericValue)

str = base64.StdEncoding.EncodeToString(cv.NumericValue)

Reference:
This modification is base on the DS SDK spec:

When EdgeX is given (or returns) a float32 or float64 value as a string, the format of the string is by default a base64 encoded little-endian of the float32 or float64 value

https://docs.google.com/document/d/1aMIQ0kb46VE5eeCpDlaTg8PP29-DBSBTlgeWrv6LuYk/edit

https://play.golang.org/p/gbQ8NpRPaHO

@cloudxxx8
Copy link
Member

this change will break the backward compatibility, so reopen it
we should fix it in 2.0

@cloudxxx8 cloudxxx8 reopened this Mar 9, 2020
@cloudxxx8 cloudxxx8 added 3-high priority denoting release-blocking issues hanoi Hanoi release labels Apr 9, 2020
@cloudxxx8 cloudxxx8 added the bug Something isn't working label Jun 29, 2020
@cloudxxx8 cloudxxx8 added this to the Hanoi milestone Jun 29, 2020
@cloudxxx8 cloudxxx8 added the hold Intended for PRs we want to flag for ongoing review label Jun 29, 2020
@tonyespy
Copy link
Member

We just discussed this in the devices WG, it turns out the spec was changed in Edinburgh from big endian to little endian. The C SDK was changed, but the Go SDK wasn't, so all floats are converted to big endian before encoding, hence the spec mismatch.

We can't change this in a 1.x version of device-sdk-go as it would break existing applications.

The current consensus is that we should probably just drop base64 encoding for v2. As such, we should make sure this gets brought up as part of our Ireland planning.

@iain-anderson iain-anderson added ireland and removed hanoi Hanoi release labels Sep 29, 2020
@iain-anderson
Copy link
Member

Leaving issue open for visibility but re-targeting for Ireland. To be edited / replaced as appropriate depending on Ireland planning outcomes.

@iain-anderson
Copy link
Member

Obsoleted by removal of base64 float encoding

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3-high priority denoting release-blocking issues bug Something isn't working hold Intended for PRs we want to flag for ongoing review ireland
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants