Implement missing form template GPS-related insertion tags #330

n2ygk · 2022-03-13T21:28:17Z

Fixes: #328

Implements several missing insertion tags for form templates (some of which are not documented other than by reading the HTML):

{GPS} - degrees and decimal minutes with Northing and Easting: 46-22.77N 121-35.01W
{GPS_DECIMAL} - decimal degrees with Northing and Easting: 46.3795N 121.5835W
{GPS_SIGNED_DECIMAL} - signed decimal degrees: 41.1234 -73.4567
{Latitude} and {latitude} - signed decimal latitude degrees: 41.1234
{Longitude} and {longitude} - signed decimal longitude degrees: -73.4567
{GridSquare} - maidenhead grid square locator: FN31cd
{GPSValid} - "YES " or "NO " to indicate GPS position is valid

Some of these values are substituted in forms but not used (like GridSquare).

n2ygk · 2022-03-13T21:29:28Z

@xylo04 I thought I'd give it a try.

n2ygk · 2022-03-13T21:39:10Z

(ignore those extra commits -- I was having issues pushing from my virtualbox...)

xylo04

@n2ygk this looks like a great first crack! Thanks for putting this together. Does it work for you locally? If so, that's super exciting!

internal/forms/forms.go

n2ygk · 2022-03-14T22:36:45Z

@xylo04 This is working properly for the GPS form but not the Winlink Check In form. Not sure why just yet other than suspecting I'm either not formatting the signed decimal string correctly or the form is buggy. The LAT gets filled in properly but the LON doesn't.

See attached before and after screen shots for the GPS Position report form. In before, the tag has not been substituted and still showed as {GPS} while in the after, it has my position.
Before:

After:

Clicking on "Click to use connected GPS LAT LON" does the right thing and fills in all the other fields on the form.

n2ygk · 2022-03-15T00:15:36Z

I got the Winlink Check In form working by reverse-engineering and finding several undocumented insertion tags which I've added in 32e7106.

There's a lot of random capitalization going on and some tags that are both supplied by the HTTP server and calculated in the form -- notably Grid Square.

xylo04

Nice improvements! Getting closer.

internal/forms/forms.go

martinhpedersen · 2022-03-15T20:22:57Z

Thanks for working on this @n2ygk 🙂

It looks like you haven't configured your editor to run go fmt? If you haven't, please remember go fmt before squashing and finalizing this PR. Thanks!

PS: We have a CONTRIBUTING.md with some guidance to first time contributors. It's not the best, but worth the read hopefully 🙂

n2ygk · 2022-03-15T21:24:25Z

Thanks for working on this @n2ygk 🙂

It looks like you haven't configured your editor to run go fmt? If you haven't, please remember go fmt before squashing and finalizing this PR. Thanks!

PS: We have a CONTRIBUTING.md with some guidance to first time contributors. It's not the best, but worth the read hopefully 🙂

@martinhpedersen: no sorry I didn't run go fmt but when I do the changes are to unrelated files:

vagrant@debian11:~/src/pat$ go fmt
http_multipart.go
riglist.go

Am I missing something? I can certainly rebase. Will do that now. Thanks for the reminder.

martinhpedersen · 2022-03-15T21:40:52Z

... but when I do the changes are to unrelated files:
vagrant@debian11:~/src/pat$ go fmt
http_multipart.go
riglist.go
Am I missing something?

These seems to be related to a change in how build tags are handled (>Go 1.17). I will fix those 👍

Running go fmt will only modify files in the current directory. You could use go fmt ./... to format sub-packages as well, but most Go developers configure their editor to run go fmt on the target file on save. Most Go-enabled IDEs will do this for you out of the box, and plain old editors like vim and emacs have various plugins to support this. (I'm a vim user myself, and use vim-go).

n2ygk · 2022-03-15T21:45:21Z

@martinhpedersen Ah. Thanks for the pointer. My editor is Emacs and my go knowledge is zero.
I found a few others besides the one I touched. Looks like the same build tags issue. I'll fix forms.go and push.

vagrant@debian11:~/src/pat$ go fmt ./...
http_multipart.go
riglist.go
internal/forms/forms.go
internal/osutil/rlimit_unix.go
internal/osutil/rlimit_freebsd.go
internal/osutil/rlimit_windows.go

n2ygk · 2022-03-15T21:52:27Z

@martinhpedersen fixed in my .emacs: (add-hook 'before-save-hook #'gofmt-before-save)"

martinhpedersen · 2022-03-15T21:58:37Z

fixed in my .emacs: (add-hook 'before-save-hook #'gofmt-before-save)"

Nice! 🙂 👍

One more thing! We generally merge PRs to develop to keep master on track with the latest release. If you don't mind, please update the base branch of this PR and rebase on top of develop. This is one of the things missing in CONTRIBUTING.md (note to self). Sorry about that.

Thanks!

martinhpedersen · 2022-03-27T13:38:26Z

@n2ygk - Are you still working on this, or waiting for a new review and/or merge?

n2ygk · 2022-03-27T15:17:18Z

@n2ygk - Are you still working on this, or waiting for a new review and/or merge?

I'm done. Waiting for review/merge.

martinhpedersen

Thanks for working on this @n2ygk 😄

I think we're almost ready to merge this. Please see my inline comments.

Thank you!

internal/forms/forms.go

martinhpedersen

Almost there!

internal/forms/forms.go

n2ygk · 2022-03-28T19:39:43Z

I suppose I should rebase this.... Standby. Done.

martinhpedersen

Since our default config provides a default gpsd address this will always throw an error for users without GPSd.

I think we need a more graceful handling of this condition than printing the standard tcp dial error each time those users fills in a form template.

Maybe we should simply omit the error if it's a network dial error?

n2ygk · 2022-04-04T13:15:50Z

Since our default config provides a default gpsd address this will always throw an error for users without GPSd.

I think we need a more graceful handling of this condition than printing the standard tcp dial error each time those users fills in a form template.

Maybe we should simply omit the error if it's a network dial error?

How about changing the default config to not list a gpsd address? It's kind of wasteful to attempt those dials irrespective of whether errors are being logged. I wouldn't want to not log errors if there is in fact expected to be a gpsd running.

martinhpedersen · 2022-04-04T18:22:41Z

I think we should avoid comprimising a good default value in our config just to keep from logging errors for users of these forms that don't care about using GPSd. The default config is very convenient for the other GPSd related features, requiring no config on most systems.

How about adding a enable_forms config option similar to enable_http? That would be a good for security as well, so we don't automatically expose a user's position without their concent.

pat/cfg/config.go

Line 227 in 587580f

EnableHTTP bool `json:"enable_http"`

For enabling use of GPSd with forms. Ref #330

martinhpedersen · 2022-04-07T14:09:26Z

@n2ygk - I've added the additional gpsd config option in 07b5665. You can either cherry-pick that and glue it together in this PR, or let me know and I'll fix the rest 🙂

Also if you could add yourself to the Contributors list if READM.md, that would be great. Thanks!

n2ygk · 2022-04-07T14:10:40Z

@n2ygk - I've added the additional gpsd config option in 07b5665. You can either cherry-pick that and glue it together in this PR, or let me know and I'll fix the rest 🙂

Also if you could add yourself to the Contributors list if READM.md, that would be great. Thanks!

Will try to get to this over the weekend. Thanks.

For enabling use of GPSd with forms. Ref la5nta#330

n2ygk · 2022-04-08T17:39:22Z

@martinhpedersen I need a little help here. I've made the change but when trying to compile and run there appears to have been some change to the web js code that throws this error: Uncaught ReferenceError: onFormLaunching is not defined I guess I'm looking for the equivalent of make install that installed not only the pat binary but the web files....

martinhpedersen · 2022-04-08T18:30:13Z

@n2ygk - I think you should rebase again, and solve that conflict in README.md.

I don't fully understand why you would get such an error. Probably not related to this PR anyway, but I will check it out once you've rebased 🙂

Thanks!!

n2ygk · 2022-04-08T18:43:44Z

@n2ygk - I think you should rebase again, and solve that conflict in README.md.

I don't fully understand why you would get such an error. Probably not related to this PR anyway, but I will check it out once you've rebased 🙂

Thanks!!

I suspect this may be related to 7850c5f which I just found now that I've rebased;-)

n2ygk · 2022-04-08T18:50:23Z

@martinhpedersen OK so this is looking like it does the right thing now. It's being very quiet about the error message per your prior recommendation so the user will not realize that the GPSd stuff needs to add "allow_forms": true. I suppose a wiki page update is warranted as well. Let me know if you'd like me to submit that.

martinhpedersen

Once last thing... (see inline comment).

martinhpedersen · 2022-04-09T06:28:34Z

internal/forms/forms.go

@@ -705,6 +710,99 @@ func (m *Manager) findAbsPathForTemplatePath(tmplPath string) (string, error) {
 	return retVal, nil
 }

+// gpsPos returns the current GPS Position
+func (m *Manager) gpsPos() (gpsd.Position, error) {


Since the error returned from this function is never used for anything but to check for success, I think we should replace this with a "ok" boolean. Then you could omit the check for "noPos" as well 🙂

All these error messages could be useful for debugging, so I suggest logging them instead using our debug logger debug.Printf.

func (m *Manager) gpsPos() (pos gpsd.Position, ok bool) { }

I'd be happy to do these last minute tweaks after merging if you prefer. Just say the word 🙂

The code currently propagates the return from NextPosTimeout() so this will become quite a bit uglier as would the NoPos checking since gpsFmt is called with a Position.

I've added a couple of debug Printf of the gpsPos error.

Feel free to make the tweaks you suggest if you disagree.

Sure, I see what you mean 🙂

As a general rule of thumb I think it's better to either log or return an error, not both. It helps with readability and makes the responsibilities clearer IMO.

I think we should merge this now, and I'll see what can be done with regards to this afterwards.

Thank you 😄

martinhpedersen · 2022-04-09T06:30:25Z

@n2ygk

I suppose a wiki page update is warranted as well. Let me know if you'd like me to submit that.

I agree! If you don't mind, yes please 😄

…the Winlink Check In form.

martinhpedersen · 2022-04-09T15:41:07Z

Thanks for working on this @n2ygk! Highly appreciated 🙂

n2ygk · 2022-04-09T16:20:01Z

@n2ygk

I suppose a wiki page update is warranted as well. Let me know if you'd like me to submit that.

I agree! If you don't mind, yes please 😄

@martinhpedersen done: https://github.com/la5nta/pat/wiki/The-web-GUI

olewsaa · 2022-06-22T17:17:22Z

I've put together a Python script that request GPS position from the Signal K server. It respond with nn.dddd nn.dddd, but I included a routine that convert the output to a grid reference. It would be benefical if Signal K could be added as a source. File is get.grid.py. Not all RPis have a local GPS attached.

xylo04 · 2022-06-22T23:48:02Z

Thanks for the suggestion, Ole! I've filed #356 to track adding Signal K as a GPS source.

xylo04 self-requested a review March 13, 2022 22:00

xylo04 suggested changes Mar 13, 2022

View reviewed changes

internal/forms/forms.go Outdated Show resolved Hide resolved

internal/forms/forms.go Outdated Show resolved Hide resolved

n2ygk force-pushed the issue328/GPS branch from 63778cb to 6ad523e Compare March 14, 2022 22:22

xylo04 suggested changes Mar 15, 2022

View reviewed changes

internal/forms/forms.go Outdated Show resolved Hide resolved

internal/forms/forms.go Outdated Show resolved Hide resolved

internal/forms/forms.go Show resolved Hide resolved

n2ygk changed the title ~~Initial proof of concept of substituting GPS var in form.~~ Implement missing form template insertion tags mostly to do with GPS. Mar 15, 2022

n2ygk changed the title ~~Implement missing form template insertion tags mostly to do with GPS.~~ Implement missing form template GPS-related insertion tags Mar 15, 2022

n2ygk marked this pull request as ready for review March 15, 2022 21:16

n2ygk requested a review from xylo04 March 15, 2022 21:16

n2ygk force-pushed the issue328/GPS branch from 7b2619f to 8075aa1 Compare March 15, 2022 21:26

n2ygk force-pushed the issue328/GPS branch from 8075aa1 to 9a16f31 Compare March 15, 2022 21:47

n2ygk changed the base branch from master to develop March 15, 2022 22:25

n2ygk force-pushed the issue328/GPS branch from 9a16f31 to dd8855d Compare March 15, 2022 22:26

martinhpedersen self-requested a review March 27, 2022 19:49

martinhpedersen requested changes Mar 27, 2022

View reviewed changes

n2ygk mentioned this pull request Mar 28, 2022

Implement all (undocumented) forms template insertion tags #338

Open

martinhpedersen self-requested a review March 28, 2022 19:18

martinhpedersen requested changes Mar 28, 2022

View reviewed changes

internal/forms/forms.go Outdated Show resolved Hide resolved

internal/forms/forms.go Outdated Show resolved Hide resolved

internal/forms/forms.go Show resolved Hide resolved

n2ygk force-pushed the issue328/GPS branch from 433ec92 to 7684ad9 Compare March 28, 2022 19:41

n2ygk requested a review from martinhpedersen March 30, 2022 22:16

martinhpedersen reviewed Apr 3, 2022

View reviewed changes

martinhpedersen added a commit that referenced this pull request Apr 7, 2022

Add gpsd.allow_forms config option

07b5665

For enabling use of GPSd with forms. Ref #330

n2ygk pushed a commit to n2ygk/pat that referenced this pull request Apr 8, 2022

Add gpsd.allow_forms config option

14b5937

For enabling use of GPSd with forms. Ref la5nta#330

n2ygk force-pushed the issue328/GPS branch 2 times, most recently from b58eee3 to 150c45b Compare April 8, 2022 18:42

martinhpedersen self-requested a review April 9, 2022 06:22

martinhpedersen reviewed Apr 9, 2022

View reviewed changes

Add several (some undocumented) insertion tags that are required for …

8a7abec

…the Winlink Check In form.

n2ygk force-pushed the issue328/GPS branch from 150c45b to 8a7abec Compare April 9, 2022 12:15

martinhpedersen approved these changes Apr 9, 2022

View reviewed changes

martinhpedersen merged commit 46a8900 into la5nta:develop Apr 9, 2022

martinhpedersen linked an issue Jun 2, 2022 that may be closed by this pull request

Implement all (undocumented) forms template insertion tags #338

Open

xylo04 mentioned this pull request Jun 22, 2022

Signal K server as a GPS source #356

Open

Implement missing form template GPS-related insertion tags #330

Implement missing form template GPS-related insertion tags #330

Conversation

n2ygk commented Mar 13, 2022 • edited

n2ygk commented Mar 13, 2022

n2ygk commented Mar 13, 2022

xylo04 left a comment

Choose a reason for hiding this comment

n2ygk commented Mar 14, 2022

n2ygk commented Mar 15, 2022

xylo04 left a comment

Choose a reason for hiding this comment

martinhpedersen commented Mar 15, 2022

n2ygk commented Mar 15, 2022

martinhpedersen commented Mar 15, 2022

n2ygk commented Mar 15, 2022

n2ygk commented Mar 15, 2022

martinhpedersen commented Mar 15, 2022 • edited

martinhpedersen commented Mar 27, 2022

n2ygk commented Mar 27, 2022

martinhpedersen left a comment

Choose a reason for hiding this comment

martinhpedersen left a comment

Choose a reason for hiding this comment

n2ygk commented Mar 28, 2022 • edited

martinhpedersen left a comment

Choose a reason for hiding this comment

n2ygk commented Apr 4, 2022

martinhpedersen commented Apr 4, 2022

martinhpedersen commented Apr 7, 2022

n2ygk commented Apr 7, 2022

n2ygk commented Apr 8, 2022

martinhpedersen commented Apr 8, 2022

n2ygk commented Apr 8, 2022

n2ygk commented Apr 8, 2022

martinhpedersen left a comment

Choose a reason for hiding this comment

martinhpedersen Apr 9, 2022

Choose a reason for hiding this comment

n2ygk Apr 9, 2022

Choose a reason for hiding this comment

martinhpedersen Apr 9, 2022

Choose a reason for hiding this comment

martinhpedersen commented Apr 9, 2022

martinhpedersen commented Apr 9, 2022

n2ygk commented Apr 9, 2022

olewsaa commented Jun 22, 2022

xylo04 commented Jun 22, 2022

n2ygk commented Mar 13, 2022 •

edited

martinhpedersen commented Mar 15, 2022 •

edited

n2ygk commented Mar 28, 2022 •

edited