SignalK / signalk-autopilot

Apache License 2.0
16 stars 14 forks source link

questions about specification #1

Open seandepagnier opened 4 years ago

seandepagnier commented 4 years ago

Set Autopilot State The value can be auto, wind, route, or standby

There needs to be other states possible

PUT http://localhost:3000/signalk/v1/api/vessels/self/steering/autopilot/actions/adjustHeading I suggest you don't do incremental heading changes. It is better the commanded heading set absolute.

Target Heading Target Wind Angle

These are redundant from above.

pypilot already defines everything needed in a tree form you could use the same paths and it would be compatible already with the smallest translation layer, but maybe Is the intention for this only to work with n2k autopilots?

tkurki commented 4 years ago

The intention is to improve the existing functionality and create how to interface with autopilots in Signal K guideline/backend agnostic implementation. Well, at least that's how I think about this, Scott can share his view....

A client can retrieve the current heading and set absolute, but why not have a convenience method for adjustHeading? Seems like a common real world use case.

The title says questions, but I found only one - something else?

Can you provide examples of other states? which do you mean

joabakk commented 4 years ago

I haven't looked into pypilot in detail, but I think it is well worth gathering what we can in one place. I am currently working on the Seatalk pilot interface. It would be excellent if we could get pilot calibration also in here with an understandable GUI. For existing pilots (N2K, Seatalk etc) we may be limited to the commands they are able to receive.

sbender9 commented 4 years ago

@seandepagnier I definitely intended this to work with other autopilots.

I think that adjustHeading makes sense from a user perspective and there are already multiple clients that use it. The AP implementation in this plugin could implement this by getting the current heading, modifying it, and then send it to the AP.

Target Heading and Target Wind Heading are already defined in the spec, so we would not change those paths.

seandepagnier commented 4 years ago

One problem with having a convenience function for adjust heading is that if the message is dropped or multiple clients collide the result is not as intended. The main issue for me is that it adds additional logic to the server which has to keep track of the absolute heading based on the relative heading changes.

If it is implemented despite this, in signalk node can I expect to just subscribe to the absolute heading value to update pypilot?

My other concern is using different fields for the compass course vs wind course etc.. What is the advantage of having too fields here? What do you mean "already defined in the spec" What spec?

I can say if it is implemented this way, each signalk autopilot client, and every translation to different autopilots will have extra lines of code to maintain the same functionallity.

@joabakk What type of calibrations do you have to implement? I can say for pypilot there are basically 3, but all are automatic except for leveling.

@tkurki How are the modes going to be enumerated then? I don't think the signalk standard supports clients enumerating enums does it? Can clients "register" values to the server and receive subscriptions from the server if any client subscribes to this? In this way they could pass the enums at registration.

Even with the ability to set the mode, there is no way to cover all of the pypilot specific settings from signalk unless the tree of values could be mounted somewhere like: vessels.self.pypilot.pilot to change the pilot from basic pid to machine learning for example but most of these fields don't exist for most autopilots.

sbender9 commented 4 years ago

The "spec" is here: https://signalk.org/specification/1.4.0/doc/ See Appendix A for defined vessel related keys.

Target Heading and Target Wind Angle are two very different things. Target Heading Magnetic is the magnetic compass heading that the AP should steer too. Target Wind Angle is the apparent wind angle that the AP should steer too (+35 degrees, -85 degrees, etc). Since these are completely different, we would never use the same path for them.

I'm not sure what you mean by "absolute heading"??

Node server is not implementing an autopilot, that's expected to been done by hardware, a plugin, or some other process. This plugin provides a common interface for client apps that act as a "control head" or for plotters, etc. that implement routing.

So, in the existing example where the AP is Raymarine hardware, a client does a PUT to "steering.autopilot.target.headingMagnetic", this is sent over N2K to the AP. When the AP gets this, it does it's thing and sends out updates (via N2K-to-SignalK) as to its current target heading. The AP is expected to maintain this. And send out updates on the current value.

I will add a default implementation for adjustHeading and make that optional for AP implementations.

It definitely seems like there's some disconnect here. I do think pypilot could be a great addition to what we have here, so hopefully we can come together on this.

thanks!

seandepagnier commented 4 years ago

On 2/5/20, Scott Bender notifications@github.com wrote:

The "spec" is here: https://signalk.org/specification/1.4.0/doc/ See Appendix A for defined vessel related keys.

Target Heading and Target Wind Angle are two very different things. Target Heading Magnetic is the magnetic compass heading that the AP should steer too. Target Wind Angle is the apparent wind angle that the AP should steer too (+35 degrees, -85 degrees, etc). Since these are completely different, we would never use the same path for them.

You are going to need "Target Heading True" for steering gps course using true north, and "Target Heading True Wind" for steering relative to true wind angle.

I am not sure why they could not all just be "Target Heading". The value would be determined by the current autopilot mode and can share the same path this is what pypilot does and it seems to work fine. It keeps clients simplified as well, but really just adds a few lines of code to have extra paths, not sure what the benefit is.

I'm not sure what you mean by "absolute heading"??

Node server is not implementing an autopilot, that's expected to been done by hardware, a plugin, or some other process. This plugin provides a common interface for client apps that act as a "control head" or for plotters, etc. that implement routing.

So, in the existing example where the AP is Raymarine hardware, a client does a PUT to "steering.autopilot.target.headingMagnetic", this is sent over N2K to the AP. When the AP gets this, it does it's thing and sends out updates (via N2K-to-SignalK) as to its current target heading. The AP is expected to maintain this. And send out updates on the current value.

I will add a default implementation for adjustHeading and make that optional for AP implementations.

would this be in the signalk node autopilot plugin then? Can it modify and output the "steering.autopilot.target.headingMagnetic" path whenever adjustHeading is set?

It definitely seems like there's some disconnect here. I do think pypilot could be a great addition to what we have here, so hopefully we can come together on this.

It will be interesting to see what opencpn can output as well. I think there are potentially a lot more values than the existing apb nmea messages which could be useful to autopilots for route following.

sbender9 commented 4 years ago

You are going to need "Target Heading True" for steering gps course using true north, and "Target Heading True Wind" for steering relative to true wind angle.

Agreed. And these can be default implementations that convert from true to magnetic or vice/versa.

I am not sure why they could not all just be "Target Heading". The value would be determined by the current autopilot mode and can share the same path this is what pypilot does and it seems to work fine. It keeps clients simplified as well, but really just adds a few lines of code to have extra paths, not sure what the benefit is.

I don't agree, it just doesn't make sense to me to have two very different things under the same path. But that's a discussion that could be had for version 2 of the specification. We can't be making breaking changes to the 1.0 version of the spec.

would this be in the signalk node autopilot plugin then? Can it modify and output the "steering.autopilot.target.headingMagnetic" path whenever adjustHeading is set?

Yes. In my raymarine implementation I can actually send this directly to the AP. But the default implementation can do the math and then PUT to steering.autopilot.target.headingMagnetic.

You can definitely add support for other settings/etc. that are not in the spec and not expected to be supported by all autopilots.

seandepagnier commented 4 years ago

It might be a problem to have "default implementations" converting magnetic headings.

The compass deviation is not enough because currents and leeway both influence the difference between compass and true heading as well as compass calibration and accuracy. So only the autopilot can make this conversion which varies all of the time rather than a simple deviation and varies depending on the mode as well as which pilot implementation is active.

On 2/5/20, Scott Bender notifications@github.com wrote:

You are going to need "Target Heading True" for steering gps course using true north, and "Target Heading True Wind" for steering relative to true wind angle.

Agreed. And these can be default implementations that convert from true to magnetic or vice/versa.

I am not sure why they could not all just be "Target Heading". The value would be determined by the current autopilot mode and can share the same path this is what pypilot does and it seems to work fine. It keeps clients simplified as well, but really just adds a few lines of code to have extra paths, not sure what the benefit is.

I don't agree, it just doesn't make sense to me to have two very different things under the same path. But that's a discussion that could be had for version 2 of the specification. We can't be making breaking changes to the 1.0 version of the spec.

would this be in the signalk node autopilot plugin then? Can it modify and output the "steering.autopilot.target.headingMagnetic" path whenever adjustHeading is set?

Yes. In my raymarine implementation I can actually send this directly to the AP. But the default implementation can do the math and then send steering.autopilot.target.headingMagnetic.

You can definitely add support for other settings/etc. that are not in the spec and not expected to be supported by all autopilots.

-- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/SignalK/signalk-autopilot/issues/1#issuecomment-582727182

sbender9 commented 4 years ago

Sure. But if an autopilot is better at it, then it can override the default implementation.

seandepagnier commented 3 years ago

I have taken a look at the "schema" for autopilotld again. I really want to come to a consensus because currently although pypilot now supports signalk data bi-directional for sensors data, it is not really practical or sensible to use signalk for controlling the autopilot with the current schema.

I believed the goal was to be able to develop interfaces for signalk-autopilot control that can work across pilots, but this is a real issue if the keys included are specific to only certain pilots. For other pilots the function will be broken, and these control interfaces will be not functional. I propose updating the schema to make it clear to developers and users what keys have universal meaning vs being specific to a certain pilot. pilots such as "raymarine" can be supported but should really be last in consideration because of their closed-source nature. The actual autopilot control (for signalk-autopilot) appears to be a photo of a raymarine remote control and I am wondering how that can be used for a universal standard and why it is choosen.

There are many keys in the steering.json schema that are simply irrelevant. For example "Operational mode" having "powersave" "normal" "accurate" Where does this come from? Again.. if it is derived from a specific pilot, I believe it is really important to put it under a key name specific to that pilot so that implementations and implementors are aware and will aid in deciphering the signalk data for users as well. For example steering.autopilot.raymarine.mode rather than steering.autopilot.mode, or steering.autopilot.n2k.mode (if all n2k pilots support it, I dont think they do) This really avoids confusion.

If I dump every relevant key to pypilot, consider autopilot.pilot (a new key) being "absolute" "autotune" "wind" "loiter" "basic" "simple" "neural" "fuzzy" and there are still other pilot algorithms. Then I have modes as well, and many parameters for each. I have more than 100 keys. Can I just put them all under autopilot. ? it would be a bit intrusive to the standard since these keys are completely irrelevant to other autopilots. It would be more reasonable to put them in steering.autopilot.pypilot.pilot or something similar if they are exposed to signalk at all.

I believe at this time, the universal keys should deal only with commanding heading, the mode of operation, and perhaps tacking. Keys like "deadzone" "backlash" "portLock" etc really should be moved to a subtree of n2k (if n2k even universally supports them) or some kind of indication that clients should consider the underlying pilot type before offering these to the user. The sooner the keys are in the correct place, will help avoid needing to change more code in the future.

As for manual control, I find keys for "rudderAngleTarget" this is fine if there is a rudder feedback sensor, but many autopilots do not have one and instead rely on relative movement. In this case, the user needs a way to command movement direction and speed of the rudder. Due to the likely hood of significant lag time in signalk networks, perhaps it should give a speed (signed) and timeout for this?

About "steering.autopilot.state" the values have "auto" "wind" "route" "standby" I believe "auto" must be renamed to "compass" to avoid confusion. It would be trivial to fix existing code at this time for this. Otherwise no one knows exactly what "auto" means or what sensor it relies on. As for "route" this a bit confusing as well, does this mean only if following a route? if we keep "route" we need a "gps" mode or a "true north" or "ground" mode to command the pilot to follow a course over ground rather than a route or compass. Also "true wind" is needed. Then there is "directControl" (isnt' this the same as standby?) "depthContour", "alarm", "noDrift" Not sure what all of these do, but there are yet more possible modes I can think of. The issue again is that most pilots will not support all of these possible states. What can we do about it? Can we provide a key to indicate which states are supported? such as steering.autopilot.available_states ? Depending on the algorithm, and available sensors pypilot supports different modes as well. I feel like this key should be renamed to steering.autopilot.mode as well, as "state" is typically reserved for something the user does not change but is feedback on the current status, such as failing to hold a course, motor stalling or hardware failure etc.

I am trying to find a generic and unbiased way to support all pilots and improve the schema. I hope we can come to an agreement on it and ideally this "demo" application could be used with pypilot. A that point it would make sense to give OpenCPN support to control autopilots without using nmea0183.

sbender9 commented 3 years ago

Definitely valid points.

available_states would make more sense to be in meta data. (And we would use camelCase)

This biggest problem with some of your proposals is they would be "breaking". And we wouldn't do that in a 1.x release of the spec. Who knows when 2.x will even be started on?

The best thing would be to expand on what's there already.

This discussion would be better in the specification repository.

seandepagnier commented 3 years ago

The case for changing it now is that in the future it will get more difficult if development continues. Currently there is very little would break because not much has been written for autopilot specific keys. It would be easy to change what little has been written now as opposed to years from now.

I also asked about version 2.x and apparently there is no plan for it and it probably will not exist for years. In the meantime version 1.x will be around for a long time.

As for expanding on what is there: it is like trying to build on a poor foundation. I tried to explain why in my previous post. The current specification is not really workable. It just does not support what is needed, and does support what isn't. This will lead to more problems in a number of ways. Why not try to correct it now so we can have a concise unbiased specification rather than wait for years for 2.x? Even then the argument will be not to change it. If we "break" it now, it will not affect any real users, so it is the perfect time.

tkurki commented 3 years ago

Could we first figure out what is needed and not concentrate on the is this breaking and needs v2? Figure out the values, their domains and their meaning first, in the abstract, and not focuse on the paths?

seandepagnier commented 3 years ago

From what I can tell there needs to be:

1) command heading 2) ability to enumerate available and set the various autopilot modes 3) perform actions like tacking, centering rudder, jibing, dodging and manual movements when the autopilot is not in control 4) extensions for specific autopilots to add more functions without cluttering the standard so that it is clear what

Do I need to move to the specification repository to continue the discussion?

Why does this repository generate n2k messages? Shouldn't the conversion from signalk to n2k be in that plugin not the autopilot one?

panaaj commented 1 year ago

Some further thoughts on the this topic have been captured here https://github.com/SignalK/specification/wiki/Extending-autopilot-related-paths-2020#autopilot-api

seandepagnier commented 1 year ago

This looks better but I suggest until the basic functions are actually implemented and working omitting these:

autopilot/deadZone autopilot/backlash autopilot/gain what subkeys should we have here? autopilot/maxDriveCurrent autopilot/maxDriveRate autopilot/portLock autopilot/starboardLock slew speeds ??

They can be added later but are problematic as they are specific to a particular autopilot and not required for basic autopilot control. If they are only used by raymarine they must be put in autopilot/raymarine//backlash for example so that users do not expect it will work on other pilots. Similarly codes used solely by pypilot would be placed in autopilot/pypilot. Can we adopt this for signalk v2?

panaaj commented 1 year ago

New PR created with the beginnings of an Autopilot API.