added local time characteristic #4

cybuzuma · 2022-05-29T21:46:19Z

cybuzuma commented

2022-05-29 21:46:19 +00:00

Motivation

InfiniTime gains the Local Time GATT Characteristic in this PR. This characteristic sends information about the current timezone to the watch and thus allows it to calculate the current UTC.

Changes

Added definitions of Local Time Characteristic
Added SetTimezone() function building the corresponding bytes.

Notes

I did not find a way to find the exact dst offset in go (there are cases where dst does not shift by a full hour). As a result, when t.IsDST() returns true, the dst offset is set to one hour and the timezone offset is decreased by one hour.

## Motivation InfiniTime gains the Local Time GATT Characteristic in this [PR](https://github.com/InfiniTimeOrg/InfiniTime/pull/1161). This characteristic sends information about the current timezone to the watch and thus allows it to calculate the current UTC. ## Changes * Added definitions of Local Time Characteristic * Added `SetTimezone()` function building the corresponding bytes. ## Notes I did not find a way to find the exact dst offset in go (there are cases where dst does not shift by a full hour). As a result, when `t.IsDST()` returns true, the dst offset is set to one hour and the timezone offset is decreased by one hour.

cybuzuma added 1 commit 2022-05-29 21:46:19 +00:00

added local time characteristic e10697448c

cybuzuma referenced this pull request from Elara6331/itd

2022-05-29 21:49:39 +00:00

added setting of timezone #16

Elara6331 commented

2022-05-30 05:34:52 +00:00

Thank you for this PR. I don't want to merge this until the InfiniTime PR gets merged, so I will keep this open for now. In the meantime, please apply my suggestion in the comment I wrote here, and modify the ITD side to match that (you can use time.Local).

Thank you for this PR. I don't want to merge this until the InfiniTime PR gets merged, so I will keep this open for now. In the meantime, please apply my suggestion in the comment I wrote [here](https://gitea.arsenm.dev/Arsen6331/infinitime/pulls/4/files#issuecomment-149), and modify the ITD side to match that (you can use [`time.Local`](https://pkg.go.dev/time#Local)).

Elara6331 referenced this pull request from Elara6331/itd

2022-05-30 05:36:52 +00:00

added setting of timezone #16

Elara6331 added the

enhancement

label 2022-05-30 05:37:20 +00:00

Elara6331 added a new dependency 2022-05-30 05:37:51 +00:00

Elara6331/itd#16 - added setting of timezone

cybuzuma commented

2022-05-30 09:05:36 +00:00

I am afraid I can not see any comment.

Elara6331 commented

2022-05-30 09:18:21 +00:00

Ok, I'll just copy them here.

infinitime.go, line 720:

Overall, this PR looks good, but this seems like it should accept a *time.Location instead of time.Time because that would be a bit better for users of the library.

Then, in the function, you can use time.Now().In(loc).Zone() because *time.Location doesn't provide a Zone() method.

infinitime.go, line 728:

This can be replaced with offset -= 3600

Ok, I'll just copy them here. infinitime.go, line 720: > Overall, this PR looks good, but this seems like it should accept a `*time.Location` instead of `time.Time` because that would be a bit better for users of the library. > > Then, in the function, you can use `time.Now().In(loc).Zone()` because `*time.Location` doesn't provide a `Zone()` method. infinitime.go, line 728: > This can be replaced with `offset -= 3600`

cybuzuma added 1 commit 2022-05-30 13:37:52 +00:00

nicer code ee06b34281

cybuzuma commented

2022-05-30 13:38:01 +00:00

Overall, this PR looks good, but this seems like it should accept a *time.Location instead of time.Time because that would be a bit better for users of the library.

Then, in the function, you can use time.Now().In(loc).Zone() because *time.Location doesn't provide a Zone() method.

Your version will return the timezone offset at Now(). You see, the timezone offset is not constant over time (see DST). That is why the timezone usually is tied to a datetime to make it unambiguous.
If now someone wants to set the watch to a different time, this might give wrong results. I agree that this is probably an unlikely corner case, but I have seen users requesting the ability to set the PineTime to a different time.
I have seen other libraries use a datetime instead of a timezone to set such information, because of the aforementioned problem.

So, I would stay with my implementation, if you can follow my argument.
But in the end: Your code, your decision.

infinitime.go, line 728:

This can be replaced with offset -= 3600

Sorry, have to admit that I was too lazy to look up if go supports this syntax. Will fix.

> > Overall, this PR looks good, but this seems like it should accept a `*time.Location` instead of `time.Time` because that would be a bit better for users of the library. > > > > Then, in the function, you can use `time.Now().In(loc).Zone()` because `*time.Location` doesn't provide a `Zone()` method. Your version will return the timezone offset at `Now()`. You see, the timezone offset is not constant over time (see DST). That is why the timezone usually is tied to a datetime to make it unambiguous. If now someone wants to set the watch to a different time, this might give wrong results. I agree that this is probably an unlikely corner case, but I have seen users requesting the ability to set the PineTime to a different time. I have seen other libraries use a `datetime` instead of a `timezone` to set such information, because of the aforementioned problem. So, I would stay with my implementation, if you can follow my argument. But in the end: Your code, your decision. > infinitime.go, line 728: > > This can be replaced with `offset -= 3600` Sorry, have to admit that I was too lazy to look up if go supports this syntax. Will fix.

Elara6331 commented

2022-05-30 18:35:52 +00:00

Your version will return the timezone offset at Now(). You see, the timezone offset is not constant over time (see DST). That is why the timezone usually is tied to a datetime to make it unambiguous.
If now someone wants to set the watch to a different time, this might give wrong results. I agree that this is probably an unlikely corner case, but I have seen users requesting the ability to set the PineTime to a different time.
I have seen other libraries use a datetime instead of a timezone to set such information, because of the aforementioned problem.

So, I would stay with my implementation, if you can follow my argument.
But in the end: Your code, your decision.

Yes, you are right. That makes me think that maybe instead of having a separate SetTimezone function, this should just be done inside SetTime, right after the time is set. That way, the timezone will always be set to local automatically unless the user specifically chooses a different timezone. That should probably also be reflected in the doc comment above SetTime().

> Your version will return the timezone offset at Now(). You see, the timezone offset is not constant over time (see DST). That is why the timezone usually is tied to a datetime to make it unambiguous. If now someone wants to set the watch to a different time, this might give wrong results. I agree that this is probably an unlikely corner case, but I have seen users requesting the ability to set the PineTime to a different time. I have seen other libraries use a datetime instead of a timezone to set such information, because of the aforementioned problem. > > So, I would stay with my implementation, if you can follow my argument. But in the end: Your code, your decision. Yes, you are right. That makes me think that maybe instead of having a separate SetTimezone function, this should just be done inside SetTime, right after the time is set. That way, the timezone will always be set to local automatically unless the user specifically chooses a different timezone. That should probably also be reflected in the doc comment above `SetTime()`.

cybuzuma added 1 commit 2022-05-30 20:34:18 +00:00

moved SetTimezone into SetTime to keep values consistent 49cde2b3c2

cybuzuma commented

2022-05-30 20:37:39 +00:00

Moved code into one function
Added bit of code documentation
Checked for existence of the new characteristic and added warning printout for it (or do you prefer it to fail completely silent?)

* Moved code into one function * Added bit of code documentation * Checked for existence of the new characteristic and added warning printout for it (or do you prefer it to fail completely silent?)

cybuzuma referenced this pull request from Elara6331/itd

2022-05-30 20:38:40 +00:00

added setting of timezone #16

cybuzuma referenced this pull request from Elara6331/itd

2022-05-30 20:40:23 +00:00

added setting of timezone #16

Elara6331 removed a dependency 2022-05-30 22:14:36 +00:00

Elara6331/itd#16 - added setting of timezone

Elara6331 commented

2022-05-30 22:30:13 +00:00

infinitime.go, line 722:

I think silent failure would be better in this case, just because many people won't have 1.10.0 at first, and printing warnings every time would be pretty annoying for users. I might re-add the warning later.

A better way to do this would be like so:

err := i.checkStatus(i.localTimeChar, LocalTimeChar)

// If the characteristic is unavailable,
// fail silently, as many people may be on
// versions older than 1.10.0. A warning
// may be added later.
if _, ok := err.(ErrCharNotAvail); ok {
	return nil
} else if err != nil {
	return err
}

infinitime.go, line 722: I think silent failure would be better in this case, just because many people won't have 1.10.0 at first, and printing warnings every time would be pretty annoying for users. I might re-add the warning later. A better way to do this would be like so: ```go err := i.checkStatus(i.localTimeChar, LocalTimeChar) // If the characteristic is unavailable, // fail silently, as many people may be on // versions older than 1.10.0. A warning // may be added later. if _, ok := err.(ErrCharNotAvail); ok { return nil } else if err != nil { return err } ```

cybuzuma added 1 commit 2022-05-31 10:01:10 +00:00

removed warning f56be08106

Elara6331 commented

2022-05-31 16:51:20 +00:00

Perfect, I'll merge this when the PR on InfiniTime gets merged. Thank you.

Ghost commented

2022-11-11 20:50:31 +00:00

First-time contributor

Hello ! We've finally merged the feature in InfiniTime! @Arsen6331 @cybuzuma thank you for your work on this feature and on ITD!