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

Add recvTime to uplink decoder input #7474

Merged
merged 1 commit into from
Jan 22, 2025

Conversation

vlasebian
Copy link
Contributor

@vlasebian vlasebian commented Jan 15, 2025

Summary

References #7467.

Changes

  • Add a recvTime field to uplink decoder input
  • Add a unit test to check the recvTime field

Testing

Steps
  1. Launch a local instance of TTS
  2. Create an application
  3. Register an end device (e.g. The Things Uno)
  4. Go to the overview of the device -> Payload Formatters tab
  5. Choose Custom Javascript formatter
  6. Update the decodeUplink function to access the recvTime passed in the input. E.g. for the default payload formatter of The Things Uno:
function decodeUplink(input) {
  var data = {};
  data.ledState = LED_STATES[input.bytes[0]];
  data.recvTime = input.recvTime.toString();
  
  return {
    data: data,
  };
}
  1. Click on the Test Decoder button
Results

Decoded test payload:

{
  "ledState": "off",
  "recvTime": "Thu Jan 01 1970 02:00:00 GMT+0200 (EET)"
}

Complete uplink data:

{
  "f_port": 1,
  "frm_payload": "AA==",
  "decoded_payload": {
    "ledState": "off",
    "recvTime": "Thu Jan 01 1970 02:00:00 GMT+0200 (EET)"
  },
  "rx_metadata": [
    {
      "gateway_ids": {
        "gateway_id": "test"
      },
      "rssi": 42,
      "channel_rssi": 42,
      "snr": 4.2
    }
  ],
  "settings": {
    "data_rate": {
      "lora": {
        "bandwidth": 125000,
        "spreading_factor": 7
      }
    },
    "frequency": "868000000"
  }
}
Regressions

None.

Notes for Reviewers

There is something really odd happening when trying to access the input directly. If I change this line from the decodeUplink function:

data.recvTime = input.recvTime.toString();

to this:

data.recvTime = input.recvTime;

I get a panic with the following message: reflect.Value.Interface: cannot return value obtained from unexported field or method, that's triggered by this piece of code:

// pkg/messageprocessors/javascript.(*host).decodeUplink
// pkg/messageprocessors/javascript/javascript.go:313

decodedPayload, err := goproto.Struct(output.Decoded.Data)
if err != nil {
    return errOutput.WithCause(err)
}

I believe it's something related to conversion of the decoder output:

(javascript.uplinkDecoderOutput) {
 Decoded: (javascript.decodeUplinkOutput) {
  Data: (map[string]interface {}) (len=2) {
   (string) (len=8) "ledState": (string) (len=3) "off",
   (string) (len=8) "recvTime": (time.Time) 1970-01-01 02:00:00 +0200 EET
  },
  Warnings: ([]string) <nil>,
  Errors: ([]string) <nil>
 },
 Normalized: (*javascript.normalizeUplinkOutput)(<nil>)
}

I'll see how it can be avoided.

EDIT: it seems like this comes from the goproto.Struct not being able to handle time.Time. Reflection is applied on the private fields of time.Time, thus the panic. Should goproto.Struct be able to handle time.Time?

Checklist

  • Scope: The referenced issue is addressed, there are no unrelated changes.
  • Compatibility: The changes are backwards compatible with existing API, storage, configuration and CLI, according to the compatibility commitments in README.md for the chosen target branch.
  • Documentation: Relevant documentation is added or updated.
  • Testing: The steps/process to test this feature are clearly explained including testing for regressions.
  • Infrastructure: If infrastructural changes (e.g., new RPC, configuration) are needed, a separate issue is created in the infrastructural repositories.
  • Changelog: Significant features, behavior changes, deprecations and fixes are added to CHANGELOG.md.
  • Commits: Commit messages follow guidelines in CONTRIBUTING.md, there are no fixup commits left.

@vlasebian vlasebian added this to the v3.33.1 milestone Jan 15, 2025
@vlasebian vlasebian self-assigned this Jan 15, 2025
@github-actions github-actions bot added the c/application server This is related to the Application Server label Jan 15, 2025
@vlasebian vlasebian force-pushed the feature/pass-recvtime-in-uplink-decoder branch 2 times, most recently from f519eb0 to 632b8e6 Compare January 15, 2025 14:09
@vlasebian vlasebian marked this pull request as ready for review January 15, 2025 14:18
@vlasebian vlasebian requested a review from a team as a code owner January 15, 2025 14:18
@vlasebian vlasebian requested a review from KrishnaIyer January 15, 2025 14:18
@vlasebian vlasebian requested a review from a team as a code owner January 15, 2025 16:28
@github-actions github-actions bot added the dependencies Pull requests that update a dependency file label Jan 15, 2025
@johanstokking
Copy link
Member

EDIT: it seems like this comes from the goproto.Struct not being able to handle time.Time. Reflection is applied on the private fields of time.Time, thus the panic. Should goproto.Struct be able to handle time.Time?

But what is the type of input.recvTime? E.g. data.recvTimeType = typeof input.recvTime ?

Can you return another standard Date, like data.now = Date() ?

@vlasebian
Copy link
Contributor Author

vlasebian commented Jan 20, 2025

The type of input.recvTime is a JS object that copies the structure of time.Time in Golang.

If I do Object.keys(input.recvTime) I get this:

add,addDate,after,appendFormat,before,clock,compare,date,day,equal,format,goString,gobDecode,gobEncode,hour,iSOWeek,in,isDST,isZero,local,location,marshalBinary,marshalJSON,marshalText,minute,month,nanosecond,round,second,string,sub,truncate,uTC,unix,unixMicro,unixMilli,unixNano,unmarshalBinary,unmarshalJSON,unmarshalText,weekday,year,yearDay,zone,zoneBounds

There is a note about the conversion of the time in the goja docs. The time is not directly converted from the Go time.Time to JS Date because Date doesn't keep location info and the time.Time zone will be lost.

I also tried what is suggested in the note for the conversion of the time, that is:

function decodeUplink(input) {
  var data = {};
  data.recvTime = new Date(input.recvTime.unixNano()/1e6);
  return {
    data: data,
  };
}

or if I simply return a Date object like this:

function decodeUplink(input) {
  var data = {};
  data.recvTime = new Date();
  return {
    data: data,
  };
}

Still, I get the same error reflect.Value.Interface: cannot return value obtained from unexported field or method.

This works indeed:

function decodeUplink(input) {
  var data = {};
  data.now = Date()
  return {
    data: data,
  };
}

but Date() called as a function and not as a constructor returns a string instead of a Date object.

Going back to converting the time.Time output, it seems to have a correct format before being converted to a Struct proto:

(javascript.uplinkDecoderOutput) {
 Decoded: (javascript.decodeUplinkOutput) {
  Data: (map[string]interface {}) (len=2) {
   (string) (len=8) "recvTime": (time.Time) 1970-01-01 02:00:00 +0200 EET
  },
  Warnings: ([]string) <nil>,
  Errors: ([]string) <nil>
 },
 Normalized: (*javascript.normalizeUplinkOutput)(<nil>)
}

I added a special case for time.Time processing in goproto.Struct that converts it to a string instead of a struct, which seems to work. The issue with this is converting from map to struct works, but going the other way around there isn't anything that marks the string as a time and when calling the struct to map converter (which I guess they should be mirrored) it will return the string representation instead of time.Time.

@vlasebian vlasebian force-pushed the feature/pass-recvtime-in-uplink-decoder branch from e03423c to 993d6ff Compare January 21, 2025 13:09
@johanstokking
Copy link
Member

Then I suggest the following:

  • Pass the recvTime as Unix nanos to the function, but the wrapper JavaScript should convert it to Date
  • If we are getting a time.Time back from goja, turn it into a Unix nano in the protobuf struct (numeric, not string)

@vlasebian vlasebian force-pushed the feature/pass-recvtime-in-uplink-decoder branch from 993d6ff to 684bcdb Compare January 21, 2025 15:15
@vlasebian
Copy link
Contributor Author

done, tested it with the test decoder:

function decodeUplink(input) {
  var data = {};
  
  data.recvTime = input.recvTime;
  data.recvTimeStr = input.recvTime.toString();
  data.recvTimeType = input.recvTime.constructor.name;
  
  return {
    data: data
  };
}

Decoded test payload:

{
  "recvTime": 0,
  "recvTimeStr": "Thu Jan 01 1970 02:00:00 GMT+0200 (EET)",
  "recvTimeType": "Date"
}

Complete uplink data:

{
  "f_port": 1,
  "frm_payload": "AA==",
  "decoded_payload": {
    "recvTime": 0,
    "recvTimeStr": "Thu Jan 01 1970 02:00:00 GMT+0200 (EET)",
    "recvTimeType": "Date"
  },
  "rx_metadata": [
    {
      "gateway_ids": {
        "gateway_id": "test"
      },
      "rssi": 42,
      "channel_rssi": 42,
      "snr": 4.2
    }
  ],
  "settings": {
    "data_rate": {
      "lora": {
        "bandwidth": 125000,
        "spreading_factor": 7
      }
    },
    "frequency": "868000000"
  }
}

I'll do an end to end test too.

@vlasebian
Copy link
Contributor Author

I connected a Things Uno and did an end-to-end test too. It works:

Screenshot 2025-01-22 at 11 40 30

@vlasebian vlasebian force-pushed the feature/pass-recvtime-in-uplink-decoder branch from 684bcdb to 74d0af3 Compare January 22, 2025 09:47
@vlasebian vlasebian merged commit 18f015f into v3.33 Jan 22, 2025
9 of 10 checks passed
@vlasebian vlasebian deleted the feature/pass-recvtime-in-uplink-decoder branch January 22, 2025 11:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c/application server This is related to the Application Server dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants