Initial alpha of Skysight weather integration #182

jfwells · 2019-04-10T16:46:57Z

Hi, this is still work in progress and not yet ready to pull, but would appreciate some feedback and advice at this stage. Let me know if there's a better place for this.

In particular, but not limited to:

I have had to make some kludges in order to get the additional libraries to build for Android (curl with SSL support, and netcdf4 and C++ bindings). For example:
- build/python/build/openssl.py: Latest versions of the openssl configure script require various environment variables to be set, including "ANDROID_HOME" and various things in PATH. I've taken ndk location from toolchain.env['ANDROID_NDK'], but I'm not clear whether this will always be available.
- build/python/build/netcdfcxx.py is likewise a kludge -- I can only get this to build with these include files. I'm sure there's a better way to do his.
I use the main "render" loop to trigger updates in Skysight (should the forecast data be updated, should the overlay be advanced). I've copied the approach used by the RASP implementation here. I'm not clear if this is the right approach, or if a timer would be better here. One particular problem is that on the ground, the initial overlay is not displayed until a render is triggered, which can be some time if GPS is not yet connected. On the flip side, when in flight, or if the map is being manipulated, the render loop gets called far more frequently than is necessary for my needs. Data downloads in background threads, but as these cannot access the map safely I need an asynchronous trigger from the main thread somewhere.
The overlays in Android and Unix are pretty much identical, but they display quite differently. The images are fairly low-res, but when zoomed seem to be interpolated nicely on Unix. On Android they just show as larger, blocky pixels. Screenshot comparison on the forum. Any points as to why this might be would be much appreciated.

There are other stylistic issues to fix (not happy with the Metric/ActiveMetric/DisplayedMetric model, and I have an over-reliance on unneeded std::strings for a start). I also need to add documentaion, but I will fix those in the next round.

Note that this requires netcdf4 and netcdfcxx4 (development) libraries to compile. On Arch these are netcdf and netcdf-cxx.

Instructions for using the implementation are here: https://forum.xcsoar.org/viewtopic.php?f=7&t=3227&p=19689#p19689

Thanks in advance for any help/advice/review.

Plantain · 2019-04-11T06:17:59Z

XCSoar developers are welcome to contact me (matthew .(at). skysight.io) if you need a SkySight account to work on or with this integration.

MaxKellermann · 2019-04-11T06:29:42Z

Before we even begin: please note that linking XCSoar with OpenSSL is illegal, because the OpenSSL license is incompatible with the GPL. This means that you can be sued for copyright violation for distributing your build: http://jfwhome.com/XCSoar-debug.apk - please take it down quickly!

Now if you really enable CURL's https features, please make that a separate commit. (And of course, choose a TLS library which is compatible with the GPL.)

The OpenSSL/GPL situation is very unfortunate, but that's what it is, and we need to obey copyright law and licenses.

Keep your branch rebased on the official master branch. Do not merge! No "Merge remote-tracking branch 'upstream/master'" commits!

MaxKellermann

Good start for this PR, but there are many many quirks and problems. I've left you a whole bunch of comments in the first round, for things that seemed obvious to me.

src/Dialogs/Settings/Panels/WeatherConfigPanel.cpp

MaxKellermann · 2019-04-11T06:43:37Z

src/Dialogs/Weather/SkysightDialog.cpp

+
+      uint64_t elapsed = BrokenDateTime::NowUTC().ToUnixTimeUTC() - m.mtime;
+
+      second_row.Format(_("Data from %04u/%02u/%02u %02u:%02u to %04u/%02u/%02u %02u:%02u. Updated %s ago"), 


Don't roll your own date formatter.

src/Dialogs/Weather/SkysightDialog.cpp

MaxKellermann · 2019-04-11T06:45:29Z

src/Dialogs/Weather/SkysightDialog.cpp

+
+  SkysightActiveMetric a = skysight->GetActiveMetric(index);  
+  if(!skysight->DownloadActiveMetric(a.metric->id))
+    ShowMessageBox(_("Couldn't update data."), _("Update Error"), MB_OK);


Can you use C++ exceptions to propagate error details and show them here?

MaxKellermann · 2019-04-11T07:08:58Z

src/Weather/Skysight/Skysight.hpp

+    std::map<tstring, tstring> GetRegions() {
+      return api.regions;
+    }
+    tstring GetRegion() {


This allocates a std::string copy for no reason. Please keep the bloat in your code smaller!

MaxKellermann · 2019-04-11T07:09:13Z

src/Weather/Skysight/Skysight.hpp

+    bool MetricExists(const tstring id) {
+      return api.MetricExists(id);
+    }
+    int NumMetrics() {


Why is this a signed integer?

MaxKellermann · 2019-04-11T07:09:32Z

src/Weather/Skysight/SkysightAPI.cpp

+  Timer::Cancel();
+}
+
+SkysightMetric SkysightAPI::GetMetric(int index) {


Why is this a signed integer?

MaxKellermann · 2019-04-11T07:09:43Z

src/Weather/Skysight/SkysightAPI.cpp

+
+
+int SkysightAPI::NumMetrics() {
+  return (int)metrics.size(); 


Why cast to signed?

MaxKellermann · 2019-04-11T07:10:22Z

src/Weather/Skysight/SkysightAPI.hpp

+
+struct BrokenDateTime;
+
+class SkysightAPI final : public Timer {


What is this class? What is a SkysightAPI?

jfwells · 2019-04-11T07:48:01Z

Before we even begin: please note that linking XCSoar with OpenSSL is illegal, because the OpenSSL license is incompatible with the GPL. This means that you can be sued for copyright violation for distributing your build: http://jfwhome.com/XCSoar-debug.apk - please take it down quickly!

Now if you really enable CURL's https features, please make that a separate commit. (And of course, choose a TLS library which is compatible with the GPL.)

The OpenSSL/GPL situation is very unfortunate, but that's what it is, and we need to obey copyright law and licenses.

Keep your branch rebased on the official master branch. Do not merge! No "Merge remote-tracking branch 'upstream/master'" commits!

Thanks. I'll hive off the Android changes into a separate commit for now, assuming that most people can build/test on machines that have curl libraries with SSL support.

Shame about OpenSSL (it looks like https support was removed from the codebase and I had wondered why). I must admit I had tried LibreSSL as getitng openSSL to compile was a bit of a pain. No luck. It looks like there are a few options: OpenSSL 3.0 (unstable), LibreSSL, BoringSSL and GnuTLS. I'll probably try them in reverse order, unless anyone has any experience or preferences that suggest otherwise.

Thanks very much for the additional comments and pointers.

MaxKellermann · 2019-04-11T07:53:44Z

Since both LibreSSL and BoringSSL are OpenSSL forks (IIRC), I think they share the licensing problem.

hor63 · 2019-04-11T10:32:29Z

The next major version of OpenSSL 3.0 is supposed to be released under the Apache 2.0 license which is compatible with the GPL. However I did not find any hint when this version is going to be released. I would not hold my breath while waiting on it.

jfwells · 2019-04-11T13:19:13Z

The next major version of OpenSSL 3.0 is supposed to be released under the Apache 2.0 license which is compatible with the GPL. However I did not find any hint when this version is going to be released. I would not hold my breath while waiting on it.

Thanks. It does seem a bit early to use v3. I'm trying to get GnuTLS to compile. It depends on Nettle which in turn needs GMP.

I can get GnuTLS and Nettle (without GMP) to compile using libs.py/Automake, but I can't (yet) get GMP to pass the "configure" step as it fails on various compiler tests. Looks like it might be messy.

hor63 · 2019-04-11T14:44:16Z

but I can't (yet) get GMP to pass the "configure" step as it fails on various compiler tests. Looks like it might be messy.

What are the error messages? GMP is a prerequisite for compiling GCC. I don't care to remember how often I configured and compiled GMP over that course. You may send me a message directly.

jfwells · 2019-06-10T13:50:36Z

I've got a working compile with WolfSSL replacing openSSL. Sorry for the force-push mess.

The code is not ready for review yet -- I have more changes to make, and to clean up the commits. Hopefully within the next couple of weeks.

lordfolken · 2019-06-11T09:26:53Z

So I got this to compile on Debian stable.

The following changes where necessary for the UNIX target on Debian:
in:

apt install libnetcdf-dev libnetcdf-c++4-dev

diff --git a/src/Weather/Skysight/CDFDecoder.cpp b/src/Weather/Skysight/CDFDecoder.cpp
index b30e101063..22dd125cf1 100644
--- a/src/Weather/Skysight/CDFDecoder.cpp
+++ b/src/Weather/Skysight/CDFDecoder.cpp
@@ -29,8 +29,8 @@ Copyright_License {
#include <netcdfcpp.h>
#endif

-#include <geotiffio.h>
-#include <xtiffio.h>
+#include <geotiff/geotiffio.h>
+#include <geotiff/xtiffio.h>
#include "SkysightAPI.hpp"
#include "OS/FileUtil.hpp"
#include "LogFile.hpp"

The files where not found in the search path as they are indeed in the subdirectory on debian. I'm not sure how one does this correctly.

diff --git a/build/netcdf.mk b/build/netcdf.mk
index 5439bb28cc..c0f50dd4d4 100644
--- a/build/netcdf.mk
+++ b/build/netcdf.mk
@@ -3,7 +3,7 @@ NETCDF = y
ifneq ($(TARGET),ANDROID)
$(eval $(call pkg-config-library,NETCDF,netcdf-cxx4))
$(eval $(call link-library,netcdfcpp,NETCDF))
-NETCDF_LDLIBS = -lnetcdf_c++4 -lnetcdf -lhdf5_hl -lhdf5 -lsz -ldl
+NETCDF_LDLIBS = -v -lnetcdf_c++4 -lnetcdf -lhdf5_hl_cpp -lhdf5_cpp -lsz -ldl
else
NETCDF_LDLIBS += -l:libnetcdf_c++.a -l:libnetcdf.a
endif

For some reason the hdf5_hl library ends with _cpp in debian.

jfwells · 2019-06-11T15:47:07Z

So I got this to compile on Debian stable.

Thanks so much for testing!

I'm not sure about the correct approach for the first issue, but for the second, I know that my NETCDF_LDLIBS lines that manually specifies the packages are temporary kludges. The pkg_config line above it should be doing the trick (I'm not sure what else needs to be done to make it so!)

lordfolken · 2019-06-19T07:52:07Z

So I'm still working on compiling the android version. Since the netcdf library is not in the toolchain, I think it needs to be added to the thirdparty.mk in the build directory. But I haven't yet managed to compile it.

lordfolken · 2019-08-10T20:28:54Z

Any progress on the implementation? :)

MaxKellermann · 2019-08-16T10:04:53Z

No progress, closing for now

closes XCSoar#222 original implementation by jfwells in XCSoar#182

This is almost identical to the code submitted in XCSoar#182. The following was changed: - no changes to thirdparty builds - remove dead includes that made the build fail

closes XCSoar#222 original implementation by jfwells in XCSoar#182

This is almost identical to the code submitted in XCSoar#182. The following was changed: - no changes to thirdparty builds - remove dead includes that made the build fail

This is almost identical to the code submitted by jfwells in XCSoar#182. The following was changed: - no changes to thirdparty builds - remove dead includes that made the build fail Further code refactoring: - Change queues to std::queue - Implement API requests as deque - Remove rvalues in SkysightCallback - Skysight/Metrics: timestamps as unsigned - Rename metric to layer for consistency - Use auto for iterator types - SkysightImageFile: remove redundant filepath - Remove SkysightImageFile - Remove LoadFromFile - Include geotiff in CDFDecoder compilation - Change datatype of displayed layer to unsigned - Remove timer inheritance - Refactored DownloadStandbyLayer and removed SetStandbyLayerUpdateState - Use new ResponseHandler implementation - CDFDecoder: Allocate arrays on heap

This is almost identical to the code submitted by jfwells in XCSoar#182. The following was changed: - no changes to thirdparty builds - remove dead includes that made the build fail Further code refactoring: - Change queues to std::queue - Implement API requests as deque - Remove rvalues in SkysightCallback - Skysight/Metrics: timestamps as unsigned - Rename metric to layer for consistency - Use auto for iterator types - SkysightImageFile: remove redundant filepath - Remove SkysightImageFile - Remove LoadFromFile - Include geotiff in CDFDecoder compilation - Change datatype of displayed layer to unsigned - Remove timer inheritance - Refactored DownloadStandbyLayer and removed SetStandbyLayerUpdateState - Use new ResponseHandler implementation - CDFDecoder: Allocate arrays on heap - CDFDecoder: Remove duplicate code

This is almost identical to the code submitted by jfwells in XCSoar#182. The following was changed: - no changes to thirdparty builds - remove dead includes that made the build fail Further code refactoring: - Change queues to std::queue - Implement API requests as deque - Remove rvalues in SkysightCallback - Skysight/Metrics: timestamps as unsigned - Rename metric to layer for consistency - Use auto for iterator types - SkysightImageFile: remove redundant filepath - Remove SkysightImageFile - Remove LoadFromFile - Include geotiff in CDFDecoder compilation - Change datatype of displayed layer to unsigned - Remove timer inheritance - Refactored DownloadStandbyLayer and removed SetStandbyLayerUpdateState - Use new ResponseHandler implementation - CDFDecoder: Allocate arrays on heap

jfwells changed the title ~~Initial alpha of Skysight weaher integration~~ Initial alpha of Skysight weather integration Apr 10, 2019

MaxKellermann suggested changes Apr 11, 2019

View reviewed changes

jfwells added 3 commits May 30, 2019 14:49

Enable SSL in cURL

7850290

bring up to date with previous submission

16bb87f

WIP

5c558ec

jfwells closed this Jun 6, 2019

jfwells force-pushed the master branch from b0de482 to ede654f Compare June 6, 2019 13:13

WIP

88f6e1f

jfwells reopened this Jun 10, 2019

lordfolken mentioned this pull request Jun 20, 2019

Implement WolfSSL Library #222

Closed

MaxKellermann closed this Aug 16, 2019

kerel-fs added the needs-work this closed pull request requires some action to be merged label Sep 18, 2019

fb added a commit to fb/XCSoar that referenced this pull request Apr 12, 2020

thirdparty/libcurl: add SSL support via wolfssl-4.3.0

1c858ea

closes XCSoar#222 original implementation by jfwells in XCSoar#182

fb added a commit to fb/XCSoar that referenced this pull request Apr 26, 2020

thirdparty/libcurl: add SSL support via wolfssl-4.4.0

0ecf3a6

closes XCSoar#222 original implementation by jfwells in XCSoar#182

fb added a commit to fb/XCSoar that referenced this pull request Apr 29, 2020

thirdparty/libcurl: add SSL support via wolfssl-4.4.0

197697a

closes XCSoar#222 original implementation by jfwells in XCSoar#182

si-gr mentioned this pull request Jul 29, 2020

Weather/Skysight: Basic integration of Skysight weather forecasts (WIP) #461

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial alpha of Skysight weather integration #182

Initial alpha of Skysight weather integration #182

jfwells commented Apr 10, 2019 •

edited

Plantain commented Apr 11, 2019

MaxKellermann commented Apr 11, 2019

MaxKellermann left a comment

MaxKellermann Apr 11, 2019

MaxKellermann Apr 11, 2019

MaxKellermann Apr 11, 2019

MaxKellermann Apr 11, 2019

MaxKellermann Apr 11, 2019

MaxKellermann Apr 11, 2019

MaxKellermann Apr 11, 2019

jfwells commented Apr 11, 2019

MaxKellermann commented Apr 11, 2019

hor63 commented Apr 11, 2019 via email •

edited

jfwells commented Apr 11, 2019 •

edited

hor63 commented Apr 11, 2019

jfwells commented Jun 10, 2019

lordfolken commented Jun 11, 2019 •

edited

jfwells commented Jun 11, 2019 •

edited

lordfolken commented Jun 19, 2019

lordfolken commented Aug 10, 2019

MaxKellermann commented Aug 16, 2019


		uint64_t elapsed = BrokenDateTime::NowUTC().ToUnixTimeUTC() - m.mtime;

		second_row.Format(_("Data from %04u/%02u/%02u %02u:%02u to %04u/%02u/%02u %02u:%02u. Updated %s ago"),


		struct BrokenDateTime;

		class SkysightAPI final : public Timer {

Initial alpha of Skysight weather integration #182

Initial alpha of Skysight weather integration #182

Conversation

jfwells commented Apr 10, 2019 • edited

Plantain commented Apr 11, 2019

MaxKellermann commented Apr 11, 2019

MaxKellermann left a comment

Choose a reason for hiding this comment

MaxKellermann Apr 11, 2019

Choose a reason for hiding this comment

MaxKellermann Apr 11, 2019

Choose a reason for hiding this comment

MaxKellermann Apr 11, 2019

Choose a reason for hiding this comment

MaxKellermann Apr 11, 2019

Choose a reason for hiding this comment

MaxKellermann Apr 11, 2019

Choose a reason for hiding this comment

MaxKellermann Apr 11, 2019

Choose a reason for hiding this comment

MaxKellermann Apr 11, 2019

Choose a reason for hiding this comment

jfwells commented Apr 11, 2019

MaxKellermann commented Apr 11, 2019

hor63 commented Apr 11, 2019 via email • edited

jfwells commented Apr 11, 2019 • edited

hor63 commented Apr 11, 2019

jfwells commented Jun 10, 2019

lordfolken commented Jun 11, 2019 • edited

jfwells commented Jun 11, 2019 • edited

lordfolken commented Jun 19, 2019

lordfolken commented Aug 10, 2019

MaxKellermann commented Aug 16, 2019

jfwells commented Apr 10, 2019 •

edited

hor63 commented Apr 11, 2019 via email •

edited

jfwells commented Apr 11, 2019 •

edited

lordfolken commented Jun 11, 2019 •

edited

jfwells commented Jun 11, 2019 •

edited