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

Automatically command rotator when tuning database entries #122

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

quentinmit
Copy link
Contributor

This required some interesting surgery to work around RequireJS bugs when loading the geodesy library.

geodesy is built as a CommonJS library, which RequireJS provides support for but it is partially broken. More detail is in the commit messages.

I could have instead patched the geodesy source or used one of the existing module converters out there, but I didn't want to add any additional build steps and build-time dependencies.

RequireJS normally allows two different types of dependencies: module
names, and URLs to JavaScript files.

When the nodeIdCompat option is set, it unconditionally removes a
trailing ".js" from dependency names even if those names are URLs,
which then causes it to try to load from the wrong URL.

Mounting tests at test/ and loading them as modules works around this
problem while still allowing nodeIdCompat to be true so node modules
can be loaded.
Some modules, like geodesy, call require() with .js filenames. In
Node, require("./dms") and require("./dms.js") are the same path, but
in RequireJS's default configuration, the former is treated as a
module path and the latter is treated as a URL relative to the
document's base path (NOT the RequireJS baseUrl!

Enabling the Node support means that all modules need to be loaded
with module names and not relative URLs, so the plugin loader is
changed to give the module name of plugin javascript.
@kpreid
Copy link
Owner

kpreid commented Dec 9, 2018

General early comments:

  • Could you explain why nodeIdCompat is necessary? I would rather not give up using actual URLs and cram everything including tests into the module-ID model.

  • Since geodesy is a dependency, there isn't much value specifically in computing the modified module dynamically. How about doing the edits inside fetch-js-deps.sh instead, which would be less complication to the web code?

  • "The server really needs to track the selected record" — indeed it does, particularly for satellites. Given the amount of fiddling needed to make this work (which, I admit, is possibly useful for other features), I'm tempted to suggest holding off on this shortest-path-to-feature and instead get that infrastructure piece in. Maybe I should work on that.

@quentinmit
Copy link
Contributor Author

General early comments:

  • Could you explain why nodeIdCompat is necessary? I would rather not give up using actual URLs and cram everything including tests into the module-ID model.

I tried to cover this in the commit messages, but the short answer is, latlon-spherical.js contains this line:

var Dms = require('./dms.js');

and RequireJS misinterprets that as relative to the HTML file and not the javascript file that includes it.

I think I could also make it work by using RequireJS's "shims" mechanism, which would save the transformation step but pollute the global namespace with LatLon and Dms objects and would need to be added to every place that calls requirejs.config; would that be better?

  • Since geodesy is a dependency, there isn't much value specifically in computing the modified module dynamically. How about doing the edits inside fetch-js-deps.sh instead, which would be less complication to the web code?

If we did anything like that we'd have to give up on geodesy being a submodule. If you want to do that I'd rather just fork it and ship the forked version with shinysdr.

  • "The server really needs to track the selected record" — indeed it does, particularly for satellites. Given the amount of fiddling needed to make this work (which, I admit, is possibly useful for other features), I'm tempted to suggest holding off on this shortest-path-to-feature and instead get that infrastructure piece in. Maybe I should work on that.

I thought about that, but on balance I went ahead with a client-side solution because it's more than enough for use with database entries, and moving targets is an edge-case.

@quentinmit
Copy link
Contributor Author

You can look at https://github.com/w1xm/shinysdr/tree/rotator-tune-shim to see what this would look like using shim instead of nodeIdCompat.

@mathisono
Copy link

-Suggestion: web ui should make a distinct visual indication between a tracking plot and Antenna position. Focus on code to update the predicted position and plot that on the globe.

Comment: importing antenna beamwidth/focal length of station properties. This effect allows directional reception rather than precise tracking. important when pairing directional dish aperture with tracking speed, and reporting this all to the web interface.

Q; Show the current predicted position ? or Current predicted position along a path? Icon moves along the ArcPath? Resulting in a checksum that returns plotting lock on for position alignment for the page.

@kpreid
Copy link
Owner

kpreid commented Dec 16, 2018

Just to set expectations, I don't expect to get around to further action on this PR until January. This is partly because of the holidays, and partly because the module loading awkwardness is tempting me to implement the """correct""" solution here (server-side continuous tracking) instead and so I may give that a shot.

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.

3 participants