SignalK / specification

Signal K is a JSON-based format for storing and sharing marine data from different sources (e.g. nmea 0183, 2000, seatalk, etc)
Other
91 stars 68 forks source link

improve autopilot specification #624

Closed seandepagnier closed 2 years ago

seandepagnier commented 2 years ago

Make the specification more generic and flexible to support more autopilots without being biased toward a specific pilot. This does not appear to break anything as keys can always be used outside the spec, but the ones removed are not currently used by any existing code anywhere.

tkurki commented 2 years ago

If we merge this the change will

First of all I don't want to break backwards compatibility and go for next major version without a very good reason. I don't quite see what the motivation here is - what practical problem are you solving with these changes?

You can't possibly know if people are using the paths that you intend to remove. This is not closed source, where the you can enumerate the affected set of software - somebody may be using those keys. I think they were originally authored by Rob, who had his autopilot implementation prior to Signal K.

Now the enum change is a catch-22. If we lose the enum and go with string there is no shared vocabulary - even if auto is pretty well established as a state value is it auto, AUTO or Auto? With the enum in place validation will fail for new values. Imho that is a better choice, as the server and all the software I know works without strict validation and passes all data, valid or not.

So unless you can demonstrate some clear advantages here I am not convinced this will not take us forward.

seandepagnier commented 2 years ago

To answer what practical problem I am trying to solve:

1) I am trying to implement full control support for pypilot by signalk. Currently it can send/receive data for wind, rudder angle, gps, and other sensors but not full autopilot control. So users must continue to use nmea and other protocols which are supported by opencpn which is ok, but more options would be nice.

2) The autopilot keys should be simplified to reflect actual functions of all pilots. Special functions of specific pilots should be in separate keys to indicate this. No one is using these keys or standard yet, but if someone tried, it would be a real problem. Currently if they write to most of these values, the results will have to be ignored which is confusing to both developers and users and results in dysfunctional programs.

As for the keys "state" and "mode" out of the 11 different enumerations, only 2 of them have any use to me. These are "standby" and "wind" I need a bunch more that are not listed. The key "auto" is generic and I need "compass" instead to indicate the pilot is on a compass course. Using "auto" is not clear it is a magnetic course, and I don't think most users will be sure it means specifically compass.

Instead of piling on a bunch of modes and supporting a small set for each different pilot, since we define keys for "windAngleTrue", "headingTrue" etc.. the mode should reflect which of these keys is used. This would make logical sense. If that is the case, there should only be 4 enumerations defined, one for each mode. I suggest a separate boolean key for standby vs enabled, but if not, "standby" could be added as a 5th possible mode for the autopilot.

For modes like "DepthContour" how can this have any place in the standard when there are no keys for what depth or contour to follow? signalk is a long way off from supporting this type of chart object data.

Furthermore, I have a clear case against the proposed enumerations besides as listed above:

These come from "raymarine autopilot". It is basically insulting to free software to base the standard on this. The word "auto" appears on their remote. The modes and states defined are based on their pilot. The signalk-autopilot used to be called signalk-raymarine-autopilot before it was renamed. raymarine has a long dark history from "raytheon" making drones, to flir producing military equipment they are a product and result of the military industrial complex responsible for much suffering around the world while concentrating wealth of a small group of people.

As a final note. Lets be honest: no one is using this standard for anything yet. There is no program or code anywhere you can find so changing it now will not break anything. If it did the break would be trivial anyway. So now is the right time to clean the standard and make it non-biased so that it can be much more useful and widespread. Once the foundation is clear it will be possible to build more, doing so in a way that does not promote proprietary systems.

tkurki commented 2 years ago

I picked windAngleTrue into #626, as I see that as a clearly needed addition.

I think the current state = wind is not quite enough now that there are two separate wind angles. Would it not make sense to set the target values separate from setting which of the three values to hold? So that I could set whichever of the three target angles and set the mode / activate which one to use separately.

You mentioned the need for "compass" state. Is it the same as headingMagnetic? Elsewhere in the SK model we have used true and magnetic to make it explicit which one we are talking about.

I agree that auto is too generic. So what if we add these states explicitly?

I suggest a separate boolean key for standby vs enabled

I think this makes sense. Then you can engage/disengage the autopilot without touching the state. How about a boolean property engaged ? Or enabled?

Can you elaborate on the implement full control support for pypilot by signalk - what else are we missing?

mairas commented 2 years ago

I am not in favor of removing any existing enumerations, due to the backwards compatibility reasons Teppo has stated. Standards should be built incrementally, to support previous or existing use cases, unless the old versions are clearly wrong. This is not the case here; the PR removes highly useful common vocabulary.

Instead, I would recommend reformulating practical concerns as additions to the specification. The addition of autopilot true wind angle (as in PR #626) is useful and obviously needed, but I didn't quite see any other issues addressed. Some of the paths are clearly implementation specific and arguably shouldn't have been included in the specification, but now that they're there, they are potentially very useful for owners of those devices. I am a happy owner of a Raymarine autopilot, and would love to see that information transmitted; they could prove highly useful in analyzing autopilot and boat behavior in different sea states.

As for the activation information (standby vs enabled as a separate key), I'm not convinced about that, either. That does not conform to the use model of any autopilot I have used, and I don't quite see the usefulness of expressing the autopilot state when the device is not enabled. Since that change would break existing implementations or create new redundancies, as in having to update both steering.autopilot.state and steering.autopilot.engaged without providing tangible benefits, I think it is better to continue using the existing state enumeration.

I do like @tkurki's proposal of adding the four new states for heading and wind angle steering modes.

seandepagnier commented 2 years ago

You mentioned the need for "compass" state. Is it the same as headingMagnetic? Elsewhere in the SK model we have used true and magnetic to make it explicit which one we are talking about.

Yes, this is correct.

I agree that auto is too generic. So what if we add these states explicitly?

  • headingMagnetic
  • headingTrue
  • windAngleApparent
  • windAngleTrue

I would agree. This gives a clear indication of which signalk key is used as well.

I think this makes sense. Then you can engage/disengage the autopilot without touching the state. How about a boolean property engaged ? Or enabled?

Yes, it is useful to be able to set the state separately. For example you could switch to wind mode, and see the wind angle displayed on the autopilot display and verify it. Also could toggle the pilot on and off without requiring individual control clients to remember the previous mode. The mode can change also with loss of data and this would be indicated even if the pilot is in standby.

Can you elaborate on the implement full control support for pypilot by signalk - what else are we missing?

Besides a lot of implementation specific keys for features specific to pypilot (which would probably go under steering.autopilot.pypilot) mostly just manual control needs to be addressed. For example, moving the ram when not engaged, as well as dodging objects while engaged. I was waiting to suggest these changes.

I am not in favor of removing any existing enumerations, due to the backwards compatibility reasons Teppo has stated. There is no backwards compatibility to maintain yet, because no program uses these. If there are, they can still use the keys, it is just not officially part of the standard, so no chance of breaking anything.

Standards should be built incrementally, to support previous or existing use cases, Again, there are no current or previous use cases.

unless the old versions are clearly wrong. This is not the case here; the PR removes highly useful common vocabulary. This version is clearly wrong as it is written based on raymarine pilots.

Instead, I would recommend reformulating practical concerns as additions to the specification. The addition of autopilot true wind angle (as in PR #626) is useful and obviously needed, but I didn't quite see any other issues addressed. Some of the paths sare clearly implementation specific and arguably shouldn't have been included in the specification, but now that they're there, they are potentially very useful for owners of those devices.

So I should put 100 pypilot specific keys under steering.autopilot as well? Why is raymarine special? Where is the source code to raymarine autopilot?

I am a happy owner of a Raymarine autopilot, and would love to see that information transmitted; they could prove highly useful in analyzing autopilot and boat behavior in different sea states.

1) if this is true, you are clearly biased toward a non-free proprietary system which impedes the progress of free software and standards. I have never used raymarine pilot and sailed all of the world. I am biased toward free-software solutions and open standards. Why doesn't raymarine pilot support signalk? If not, then why do they get special consideration? Why should signalk be obfuscated?

2) If you want that information it can be defined here: steering.autopilot.raymarine.*

This way users and developers won't expect any of it to work for anything besides a raymarine pilot.

tkurki commented 2 years ago

@seandepagnier please understand that the current spec is not a result of a particularly developed grand design or policy. More like let's add definitions for stuff that we know of. Especially in the early days, when there was no or very little SK software that was usable and where you could use the stuff and learn what works and what doesn't.

Also bear in mind that one goal of Signal K is interoperability and providing a common data model. In this respect it is useful to be able to provide representation for data from actual products that people use.

Also there is very little manpower available for specification work. It is tedious and not very rewarding. Often the driver can be I want my stuff to work. This produces bias and gaps in the coverage.

So Raymarine is in no way fundamentally special, just accidentally special.

I don't think we should add casually paths that are specific to a certain product/system/software, but create a framework where they can be presented.

Let's imagine you come up with a great machine learning autopilot algorithm called randomWave that takes parameters feedbackType (enum), feedbackRatio (float 0..1) and gamma (integer 0..65535). Then we add those under steering.autopilot.pypilot.randomWave.

I try pypilot and experience the superiority of randomWave, but not being a fan of Python I reimplement it as part of my rustPilot. Why would the params still be under pypilot? Why should pypilot be special? Shouldn't we create a structure that says "this autopilot supports algorithm randomWave that takes these parameters typed so and so"?

We can also think about windAngleTrue discussed above. It is not universal, I don't think my tillerpilot supports it. Following your logic (as I understand it) it should go under each vendor branch, or more appropriately under model specific branches, one for each autopilot that supports it.

Another real life example is navigation.speedThroughWaterTransverse. I think there's only one product in the market that produces this value, so when adding it should it have landed under a vendor specific branch? Or a path that describes what it is, not what particular sensor or product it came from.

seandepagnier commented 2 years ago

I could explain my rational in that defined universal paths based on nature and physics make sense in the standard as they have a concrete meaning. This would include traverse water speed (I am developing 3d printed sensors to measure this) and true wind angle, but does not include "backlash" or "deadzone" since these are made up human ideas with no defined units or universal known meaning that can be applied across different autopilots and expect the same result for the same values. These values will always have to be reset and changed if the autopilot were to change.

As for the hypothetical re implementing the algorithm (to be) in rust: This would be really great, but I really doubt the performance would be the same in both pilots and so again, unlike something based on a physical measurement the values needed would be completely different and could change with software changes so do not really think sharing configuration values would be desirable, at least the benefit of doing so would also be minimal at best. m Part of the reason I want to remove many of these paths, is they are broadcasting a particular ideology for how to implement an autopilot. For example the "deadzone" is in space, where pypilot's "period" is in time. I found a similar frustration dealing with route following where cross track error is computed. This XTE term is how many autopilots function but it's a terrible algorithm and the wrong way to implement route following in every case, yet it is already (unfortunately) proliferated through many standards as a sub-optimal method. It is sort of like the windows operating system: It is unfortunate for everyone that it ever existed.

If despite this, stubbornly anyway you will not remove these keys even though no one has used them yet, even though they are specific to a proprietary system, and even though they indicate (to me anyway) an inferior algorithm and even though, they could simply be used outside the specification leaving the specification cleaner and more understandable, then I would like to add just as many, maybe quite a few more (say 25 or so) new keys that are specific to pypilot under steering.autopilot. I can't reuse any of the existing keys such as maxDriveRate since this is in radians and pypilot uses percentage (since it can work without rudder feedback) I can't use maxDriveCurrent, instead I need maxCurrent because this is simply a software fuse, the controller does not regulate current as the spec currently indicates it does for maxDriveCurrent. There are 7 or 8 other key configuration terms then I also have a lot of gains (about 15 or so and growing) because I have several pilot algorithms to choose from. None of these values will ever interoperate with other autopilots most likely even if they used similar logic as the units are arbitrary. So these are all more or less the same as the keys I suggested we remove from the specification.

If this is ok I can make a pull request to add a bunch of keys (perhaps tripling the size of steering.json) to give basic control and configuration and at least then the spec will not be "biased" toward the raymarine pilot. I still prefer that implementation specific keys without real world units, and the keys that are specific to specific (inferior) algorithms to not be in the standard at all or at least in their own path, but ultimately it is up to the owners of the signalk repository here to decide which course is the right one.

I am beginning to realize that signalk is not about making a concise standard as I would prefer, but more like a soup where any available ingredient is added. Interestingly this parallels the dilemma (fork) of bare boat necessities vs lysmarine/openplotter. Neither way is right or wrong, just a different idea.

tkurki commented 2 years ago

I think it would be fruitful to list the keys you'd like to see added in an issue first, as JSON schema is pretty tedious to iterate with.

I am closing this one - if you feel there's something still open in this PR feel free to reopen.