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

WeatherService #353

Merged
merged 42 commits into from
Dec 9, 2021
Merged

WeatherService #353

merged 42 commits into from
Dec 9, 2021

Conversation

Avamander
Copy link
Collaborator

@Avamander Avamander commented May 14, 2021

This PR aims to implement a WeatherService for both short-term and long-term forecasting.

Currently implemented:

  • - QCBOR tested and works stable
  • - BLE service itself
  • - Data is received
  • - Fully working decoding and the parsing should detect/avoid most common mistakes
  • - Getters for any watchapps/faces that want the data (Improved API for watchfaces according to what @kieranc suggested)
  • - Developer only textual UI, accessible with a small modification, it's not for end-users (rebasing on latest develop made it work when running mcuboot-app, don't ask me why)
  • - Full mapping of Gadgetbridge's weather provider to InfiniTime's
  • - Described the data format
  • - Testing with real data
  • - Update docs

Needs more work:

  • - Waiting for companion feedback

Not in scope:

  • Limit std::vector size, implement a purge algorithm for still valid events over set size
  • Fully fleshing out the control channel (the timeline is self-cleaning right now, so very little use atm)
  • Nice UI
  • Watchface integration

Fixes #208

@Avamander Avamander force-pushed the patch-weather branch 3 times, most recently from 56bb44f to d1eab7f Compare June 15, 2021 20:02
@Avamander Avamander force-pushed the patch-weather branch 2 times, most recently from 73e4b0a to dae3932 Compare June 22, 2021 19:15
@Izaic

This comment has been minimized.

@Avamander Avamander changed the title WIP: WeatherService WeatherService Nov 28, 2021
@Avamander Avamander added feature request needs more work This PR needs more work needs testing Needs testing on a physical device labels Nov 28, 2021
@Avamander
Copy link
Collaborator Author

Needs a bit of bug squashing, then it's quite ready for companion apps.

Some really neat watchface etc. integrations can come later.

@Avamander
Copy link
Collaborator Author

Writing data to the watch should now be fully testable for those wishing to develop a companion.

@Avamander
Copy link
Collaborator Author

I guess @Arsen6331, @alexr4535, @piggz you could try and take a look and see if (currently only) pushing data is sufficient and clear for you, or if you find any bugs.

@Elara6331
Copy link
Contributor

Can you please provide the format of the data I need to write, and to which characteristics? I tried to follow the code to figure it out, but the most I've done in C++ is hello world, so I cannot figure it out.

@Avamander
Copy link
Collaborator Author

@Arsen6331 Check out src/components/ble/weather/WeatherData.h#20 and src/components/ble/weather/WeatherService.h#111

@Elara6331
Copy link
Contributor

Ok, I got the characteristics. How do I enable the screen to see what I am actually doing?

@Avamander
Copy link
Collaborator Author

Avamander commented Dec 4, 2021

@Arsen6331

Ok, I got the characteristics. How do I enable the screen to see what I am actually doing?

I accidentally commited the changes that enables it under Navigation's icon, but now you can nicely see them here: 86e21f5

I don't want to make it visible by-default because it's not an end-user app for displaying weather.

@Avamander
Copy link
Collaborator Author

@Zorvalt I'd love to discuss building #790 onto the timeline once this PR is merged, would you (or anyone reading here) be interested?

@Elara6331
Copy link
Contributor

I am trying to get it to accept my CBOR. This looks right to me, but it returns an error. Am I doing it incorrectly? This is for precipitation:

A5 // Object (5 pairs)
    64 // UTF-8 string (length 4)
        74797065 // "type"
    05 // positive integer: 5
    66 // UTF-8 string (length 6)
        616D6F756E74 // "amount"
    1850 // positive integer: 80
    67 // UTF-8 string (length 7)
        65787069726573 // "expires"
    190E10 // positive integer: 3600
    69 // UTF-8 string (length 9)
        6576656E7454797065 // "eventType"
    01 // positive integer: 1
    69 // UTF-8 string (length 9)
        74696D657374616D70 // "timestamp"
    1A61AB49EB // positive integer: 1638615531

@Zorvalt
Copy link
Contributor

Zorvalt commented Dec 4, 2021

Yes, absolutely @Avamander :-)

@Avamander
Copy link
Collaborator Author

@Arsen6331 Capitalize the keys, I think

@Elara6331
Copy link
Contributor

@Arsen6331 Capitalize the keys, I think

Ok, that works. Thank you. You should add that to the comment in src/components/ble/weather/WeatherData.h

@Elara6331
Copy link
Contributor

I have all the events working. This is actually very simple once I figured out what to do.

@Avamander
Copy link
Collaborator Author

@Arsen6331 Anything you miss? Manipulations you'd like to do on the timeline from the companion side?

@ItzSwirlz
Copy link

Just make sure there is a way to have it only run/work/be enabled when Gadgetbridge is active and linked. (InfiniLink is still in development for IOS stuff)

@Elara6331
Copy link
Contributor

@Arsen6331 Anything you miss? Manipulations you'd like to do on the timeline from the companion side?

This is enough for me to work with. There is some data I may not be able to fill, but most of it should be fine. I've implemented it in my library locally, so I can test any changes and such easily. I will also be trying to implement it locally in ITD so I can see what it would take. Thank you.

@Avamander
Copy link
Collaborator Author

@ItzSwirlz

Just make sure there is a way to have it only run/work/be enabled when Gadgetbridge is active and linked. (InfiniLink is still in development for IOS stuff)

If left unpopulated, the components fetching data from the Timeline should check for reserved and zero values and gracefully fall back.

@Avamander Avamander added this to the 1.8.0 milestone Dec 6, 2021
@JF002 JF002 merged commit d29a981 into InfiniTimeOrg:develop Dec 9, 2021
@Avamander Avamander deleted the patch-weather branch December 9, 2021 21:35
@Avamander
Copy link
Collaborator Author

Companion developers please kindly version-lock your support to 1.7.1 to 1.8.0.

If all goes well we don't have to change anything in Weather (only release the lock), but if we do, companions won't break when InfiniTime changes.

@piggz
Copy link
Contributor

piggz commented Dec 13, 2021

Thought ... all the Timeline classes ... as theyre just public data members, wouldnt it be better to make them structs? Only difference is structs are public by default and youre setting everything public anyway.

@Avamander
Copy link
Collaborator Author

@piggz In theory one could, but I don't see much benefit from that. It's just limits our options a bit, preemptively. Structs don't allow constructors for example, and it'd be a bit annoying to go changing it around once that is determined to yield nicer code.

@piggz
Copy link
Contributor

piggz commented Dec 14, 2021

Actually they do ... in c++the only difference between structs and classes is the default visibility. The syntax for constructors is the same :) depends if you think there will be functions added, and private members, of if theyre just structures used for data transfer.

@Avamander
Copy link
Collaborator Author

@piggz

Actually they do ...

Ooh, my misconception.

depends if you think there will be functions added, and private members, of if theyre just structures used for data transfer.

Not out of the question. The first thing that comes to mind is that someone might wish to add a getFahrenheit() to Temperature or getBeaufort() to Wind.

In the end it should be fine?

@devnoname120
Copy link

@Avamander Which app do you use as a provider? The weather is disabled for InfiniTime on the official GadgetBridge.

@Avamander
Copy link
Collaborator Author

@devnoname120 Upstream GB doesn't have weather sending-remapping at the moment, I wrote my own. I couldn't upstream it because I couldn't get TWG to send weather intents (I can't test receiving actual intents).

Siglo has some support available I think.

@Elara6331
Copy link
Contributor

Elara6331 commented Jun 19, 2022

ITD also sends the weather every hour, or upon running a command, for any location you configure in the config file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request needs more work This PR needs more work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can weather support be added?
8 participants