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

Restructured to allow sentences to be loaded from files #8

Closed
wants to merge 1 commit into from

Conversation

ieb
Copy link

@ieb ieb commented Sep 5, 2017

Apologies for the size of the patch.
This restructures the module into 1 file per sentence and adds some NKE performance related sentences that are understood by the NKE app. The performance data is calculated in SignalK/signalk-derived-data#7

The calculations have been checked and look ok but might have errors.

'N'
]);
function mapToNmea(encoder, name) {
if ( encoder.ttl === undefined) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did I understand this right: none of the encoders have ttl defined and it can only be used in encoders, not user defined?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the intention was that the encoder could define it if required. I didnt find I needed to reduce the frequency on my network. I guess it could be user defined as an alternative. Things like pressure and temperature don't need to be ever 2s.

@tkurki
Copy link
Member

tkurki commented Sep 7, 2017

There are several small things with your PR that I thought I'd sooner fix myself. Stuff like copypasted function being name mwv in HDM.js, global variables leaking, bug in fixAngle etc.

I created a new PR on top of the recent changes in master #10. It has all the new conversions, but not the skipduplicates etc, which I would like to introduce properly (user configurable) instead of being there but not being used at all. The commit message also refers to this PR.

I hope this is ok with you @ieb ?

@ieb
Copy link
Author

ieb commented Sep 7, 2017

yes, lgtm

@tkurki tkurki closed this Sep 7, 2017
tkurki added a commit that referenced this pull request Oct 9, 2017
Additional conversions by https://github.com/ieb from
#8.

chore: remove unused dummy file
tkurki added a commit that referenced this pull request Oct 9, 2017
Additional conversions by https://github.com/ieb from
#8.

chore: remove unused dummy file
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.

2 participants