Skip to content

support per-field transformations#359

Open
patrickdemers6 wants to merge 1 commit intoteslamotors:mainfrom
patrickdemers6:datum-transformations
Open

support per-field transformations#359
patrickdemers6 wants to merge 1 commit intoteslamotors:mainfrom
patrickdemers6:datum-transformations

Conversation

@patrickdemers6
Copy link
Collaborator

@patrickdemers6 patrickdemers6 commented Feb 12, 2025

Description

Introduces the ability to define per-field transformations.

Transformation functions receive the device version, allowing a transformation to only impact specific vehicle versions.

Type of change

Please select all options that apply to this change:

  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Bug fix (non-breaking change which fixes an issue)
  • Documentation update

Checklist:

Confirm you have completed the following steps:

  • My code follows the style of this project.
  • I have performed a self-review of my code.
  • I have made corresponding updates to the documentation.
  • I have added/updated unit tests to cover my changes.
  • I have added/updated integration tests to cover my changes.

@patrickdemers6 patrickdemers6 force-pushed the datum-transformations branch 2 times, most recently from 7b85230 to a28760d Compare February 12, 2025 18:14
)

var (
pacificTimeLoc *time.Location
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: pacificTimeLocation

func transformTimestamp(d *protos.Datum, _ semver.Version) {
if timestamp := d.GetValue().GetDoubleValue(); timestamp != 0 {
_, offset := time.Now().In(pacificTimeLoc).Zone()
d.Value = &protos.Value{Value: &protos.Value_DoubleValue{DoubleValue: timestamp + float64(offset)}}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this correctly handles daylight savings transitions, because it's getting the offset of the current time then applying it to a time that may be on a different day?

Then again, I don't know how the vehicle handles this situation either.

@patrickdemers6 patrickdemers6 force-pushed the datum-transformations branch 2 times, most recently from aaf6206 to 9bf61d6 Compare February 12, 2025 22:36
@patrickdemers6
Copy link
Collaborator Author

Ok this actually isn't going to work at all since we need the vehicle's timezone. Working on a firmware fix.

@jbanyer
Copy link

jbanyer commented Feb 13, 2025

Ok this actually isn't going to work at all since we need the vehicle's timezone. Working on a firmware fix.

I don't understand this comment. As reported in #251, it appears that interpreting the timestamp in Pacific Time then using the result at a vehicle local time works. If so, converting from Pacific Time to UTC should work, except perhaps for the daylight savings issue I mentioned?

I'd like to fully understand this issue because I'd like to use prefer_typed before waiting for a firmware fix that will likely take many months to roll out to Australian vehicles. Cheers.

@patrickdemers6
Copy link
Collaborator Author

Converting to UTC will yield a time in the local vehicle timezone, but there's no way for me to give a true unix timestamp from the backend. Adding a conversion here that still gives a unix timestamp that is wrong doesn't feel like the right move. Open to your thoughts of course though.

@patrickdemers6
Copy link
Collaborator Author

I do think adding the ability for per-field transformations by version is still valuable, but maybe not for time.

@patrickdemers6 patrickdemers6 changed the title support per-field transformations, fix datetime offset support per-field transformations Feb 13, 2025
@jbanyer
Copy link

jbanyer commented Feb 13, 2025

FWIW, in our case, for ScheduledChargingStartTime, what we actually want is to to determine if the vehicle is currently in the 6 hour window that follows the scheduled start time. We are currently doing this using these inputs:

  • the scheduled start (local) time-of-day, eg "02:30", which we are deriving from ScheduledChargingStartTime
  • the vehicle's local time, which we derive using our own setting for the vehicle's timezone

We don't actually need the date-and-time of the next (or current) scheduled charge, we just need the time-of-day, so we don't need a complete timestamp.

An even better field for us would be an indicator that the vehicle is currently in the 6 hour schedule window or not, since that is actually all we care about. (We are trying to detect if a charging vehicle may be charging due to the schedule)

@stx
Copy link

stx commented Feb 15, 2025

SoftwareUpdateScheduledStartTime has the same issue.

I think reporting a Unix UTC timestamp (regardless of how it's achieved) is the most friendly and sane thing to do. But considering this is already in the wild, and affecting multiple fields, maybe leave this in as a systematic quirk and tell devs to interpret all fleet telemetry timestamps as America/Los_Angeles.

@stx
Copy link

stx commented Feb 15, 2025

Actually, I don't think timezone interpretation fixes this. Example with SoftwareUpdateScheduledStartTime:

Fleet API: 1739692800553 - Sunday, February 16, 2025 8:00:00.553 AM GMT - Correct
Fleet Telemetry: 1739700120 - Sunday, February 16, 2025 10:02:00 AM GMT - Wrong. How would you convert this to get the above???

@jbanyer any guidance here? Some simple JS code or something would help understand how you're fixing this.

@stx
Copy link

stx commented Feb 15, 2025

Example for ScheduledChargingStartTime...

Fleet API: 1739650500 - Saturday, February 15, 2025 8:15:00 PM GMT - Correct
Fleet Telemetry: 1739682900 - Sunday, February 16, 2025 5:15:00 AM GMT - Wrong

@bperlman
Copy link

Converting to UTC will yield a time in the local vehicle timezone, but there's no way for me to give a true unix timestamp from the backend. Adding a conversion here that still gives a unix timestamp that is wrong doesn't feel like the right move. Open to your thoughts of course though.

@patrickdemers6 - my 2 cents would be that I agree that sending a unix timestamp that is still wrong is likely the wrong move. I struggle to think of any reason that anything other than a correct unix timestamp would be the 'right' move here.

Given the need to work around this pending a firmware update, it would be ideal if we could get some kind of notification when this is changed and at what firmware version, but it should absolutely be changed IMO. I'd be willing to bet that pretty much nobody would expect a unix timestamp to have been derived improperly and therefore not be able to be implicitly trusted. 🤷🏻‍♂️

Workaround (which we'll use for now) seems possible provided the vehicle's timezone is known (at least I can't think of a way to do it without that given the inputs that would have been used to create the incorrect timestamp in the first place).

@jbanyer
Copy link

jbanyer commented Feb 18, 2025

@stx the way we use the ScheduledChargingStartTime field is perhaps not what you'd expect. As I tried to explain above, we actually only want the local time-of-day of the schedule start, ie the equivalent of scheduled_charging_start_time_minutes in the vehicle data API. We are not trying to obtain the equivalent of scheduled_charging_start_time, although we may end up doing that below.

Per the example from #251:

  • Vehicle schedule start time: 00:15 (15 minutes past midnight)
  • vehicle data scheduled_charging_start_time_minutes: 15 (15 minutes past midnight)
  • telemetry ScheduledChargingStartTime: 1736756100

Unix time 1736756100 interpreted using various timezones is:

  • GMT: Mon Jan 13 2025 08:15:00 GMT+0000
  • vehicle local time (Australia/Sydney): Mon Jan 13 2025 19:15:00 GMT+1100
  • LA time: Mon Jan 13 2025 00:15 GMT-0800

The "LA time" interpretation gives the correct time-of-day for the vehicle, and that's all we need. We combine this with the vehicle's timezone (which we obtain separately from the user, not from the vehicle) in order to obtain an actual timestamp.

@stx
Copy link

stx commented Feb 21, 2025

@patrickdemers6 In addition to the timezone mismatch, it seems like there's an additional 2-minute offset happening:

Scheduled time: Friday, February 21, 2025 11:55 AM PT
Fleet API scheduled_time_ms: 1740167701092 (Friday, February 21, 2025 11:55:01.092 AM PT)
Fleet Telemetry ScheduledUpdateTimeMs: 1740167821 (Friday, February 21, 2025 11:57:01 AM PT)

Appears to be exactly 2 minutes ahead no matter which time is set.

edit: looks like this is because Fleet Telemetry is including the additional warning countdown time and Fleet API isn't.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants