Noiredd / PEGAS

Powered Explicit Guidance Ascent System - a KSP & RO autopilot using the Space Shuttle guidance algorithm, UPFG
http://forum.kerbalspaceprogram.com/index.php?/topic/142213-pegas-powered-explicit-guidance-ascent-system-devlog
MIT License
114 stars 31 forks source link

Custom steering vector transform #38

Open Noiredd opened 3 years ago

Noiredd commented 3 years ago

It has been pointed out that simple steering locked to a vector calculated by UPFG breaks for some vehicles. The solution is to apply a vehicle-specific transformation of the steering vector that accounts for the vehicle center of mass, and then locking the steering to that.

This should be easy to implement using an additional layer of abstraction between UPFG output and kOS cooked steering. An additional function would input the raw UPFG steering vector and output a real vector that steering would be then locked to. This function would default to an identity transform:

FUNCTION nullTransform {
    DECLARE PARAMETER inputVector.
    RETURN inputVector.
}

But it could be overridden by the user in the form of a delegate, for example passed through the CONTROLS lex.

Seems like a simple fix, but help with testing would be appreciated.

Jakathan commented 3 years ago

So I prototyped this out a bit in my own repository. I'm going to do some testing on it with both the null transform and my own transform function over the next few days. I'm not very familiar with GitHub, so I'm not really sure how to best show you my changes though. Never really done any sort of collaborative coding before.

lapiante commented 3 years ago

Please excuse my poor coding skills, if i understand correctly, i have to add these lines below the current lines. Is that correct ?

I would be happy to help you but only for tests. sorry :)

Jakathan commented 3 years ago

No, it's going to take more than just that. I'm going to do some testing and then open a pull request that should cover the needed changes, at least as I have them prototyped. I'm sure Noiredd will have some changes/suggestions once I do before it's ready.

Noiredd commented 3 years ago

@lapiante For now the functionality isn't even implemented, so using these few lines won't do anything.

@Jakathan Before we rush to coding and PRs, let's figure out how exactly we want to handle this. Initially I was thinking that I'd implement the PEGAS-side feature, but that might take me a few days. If you want to do all that - fine with me. But the more important decision is regarding the actual delegate: should PEGAS come equipped only with the most basic version (i.e. nullTransform), leaving it for the user to implement their own transforms, or is it possible to provide a built-in transform? What's your opinion on this?

Jakathan commented 3 years ago

I guess that's a question for you to answer, as it's your project 😄

I'm more than happy to let you have my steering transform function to include with PEGAS. I think that would be the most common one people might have use of, as well as the one that should be most plug-and-play. It would also be a good example of what one could do with the functionality.

As far as switching transforms mid flight, shouldn't it be as easy as using a delegate event in your sequence to change the steering transform variable to whatever new transform you want?. You could also create a specific event for that, but I don't think that's necessary.

Noiredd commented 3 years ago

I guess that's a question for you to answer, as it's your project

It's my project, but since there are others using it besides me, I feel that some changes - especially those that come suggested by the community - should be discussed with this community :)

It would also be a good example of what one could do with the functionality.

This gives me an idea. What if the bare functionality (i.e. delegate handling) but no delegates themselves were implemented in PEGAS directly, but an example usage - i.e. your function - be included in the documentation? This way the "framework" code would not bloat, but users with these particular needs could still have what they want.

As far as switching transforms mid flight, shouldn't it be as easy as using a delegate event

It feels a bit like a monkey-patch to me :P But I suppose that should work. I'd expect a very small fraction of users to ever need that to begin with, so creating a whole new facility (like a specific event just to handle this) feels like a waste of time (and a code bloat).

Alright I'm going to implement the base feature the next time I've got any free time, then we'll talk about including your example, @Jakathan - I think it would be good if you authored a commit including this, to take your deserved credit.

Jakathan commented 3 years ago

What if the bare functionality (i.e. delegate handling) but no delegates themselves were implemented in PEGAS directly, but an example usage - i.e. your function - be included in the documentation?

I think this is a great solution! This gives a practical example of how you could use the functionality without complicating it too much for someone who just needs the base code. It would also give an opportunity to reiterate the need to avoid dead delegates, like you do in the Sequence tutorial section. Let me know when you have the delegate handling implemented and I can start writing some example documentation.

Noiredd commented 3 years ago

@Jakathan I've just came up with a concrete idea for addons for PEGAS. Please check out #33 and tell me what you think of it in the context of this transform. Maybe we could get away without building it into the main code itself, but instead it could live as an addon? Here's what I'm thinking:

FUNCTION steeringVectorTransform {
    // override previously computed steering vector with the transformed version
    SET steeringVector TO shuttleSteerDir(steeringVector).
}
// have this ran after every iteration of UPFG
register_hook(steeringVectorTransform, "activePost").

Following what I outlined in #33, this would make your function be called after every iteration of UPFG (about this line, for example). The only potential problem I see with this approach is that there would be some non-zero delay between UPFG calculating the base steering vector (bear in mind, steering is locked to this variable) and recalculation by the post-loop hook. I think this time would be negligible (i.e. not enough for kOS to start rotating the vehicle to the pre-correction vector before the correction kiks in), but maybe I'm wrong. If it worked however, it could be a nice use for the addon mechanism without burdening the existing codebase and, especially, documentation :)

Let me know what you think!

Jakathan commented 3 years ago

I quite like this approach. I looked over your comments in #33 and it seems to be a pretty elegant solution, especially as a general way to add in custom modules.

I haven't examined script timing too closely yet, so I'm not sure how quickly the loop runs, but I agree that it will probably be negligible. Upping the IPU value would also help prevent that somewhat, as you're nowhere near maxing that out yet. When you get it implemented, I will test it out a little bit. I'm going to try and launch a rocket with the probe core pointing backwards, so any delay between the initial and corrected steering should be more obvious.

Also just to prove that I can...

Noiredd commented 3 years ago

Addons are here! Now we only need to discuss one thing: should this functionality be highly configurable (i.e. using the above proposed system involving pulling a delegate from vehicle or mission configs), or would it suffice to just have your shuttleSteerDir packed into the module? What do you think?
Personally I'm leaning towards simplicity, since as far as I understand your need for this, the latter approach should cover your use case without being overly complicated.

In either case, I'd really like to leave this up to you - whatever you implement that works for you, I'd invite you to share it as an addon in the addon repo :)

Jakathan commented 3 years ago

I'm not sure exactly which module you're referring to in the 2nd approach.

As far as implementing this as an addon, I've rewritten it so that it can be used as such and have done a bit of testing. I need to write the documentation for it, but once I get a chance to do that, I can add it to the addon repository. Although Patrykz94 already beat me to the first addon...

Noiredd commented 3 years ago

In the first approach, this addon would retrieve the vector transforming delegate from controls (for example) and then just call that. It'd be the user's work to write the delegate and include it in their vehicle descriptions. In this case it'd be nice if the addon came with some example on how to write it, otherwise it'd be pretty difficult to understand even.

The second approach comes with the transformation function already built in. So the vehicle description lexicons do not have to include it, it's easier on the user. It could still be configurable (so that one doesn't have to install/uninstall the addon between different flights), e.g. enable the transformation if controls:haskey("needTransform"), otherwise use the null transform that returns what it takes. I guess the thing I might not be understanding perfectly is whether there's a need to use different kinds of transforms, or does shuttleSteerDir handle everything always.

And don't worry about not being the first ;) Patryk's addon is a culmination of the idea he proposed as far back as 2017, then implemented as a built-in feature proposal in 2020 - I guess it's fair that his idea got there first :)

lapiante commented 2 years ago

Hello, I'm back on KSP RO/RSS and I want to know if we can launch the space shuttle whith ''PEGAS'' ?

Noiredd commented 2 years ago

@lapiante No, not yet.

@Jakathan If you're still interested in making this happen, I have a perfect and simple idea to solve this as an addon. It would consist of 2 hooks. One remains the same as a few comments up, something bound to "activePost" that calculates a new steering vector given some transform, but with a little twist: instead of storing the result back into steeringVector, it would write to a new global variable (owned by the addon). Second hook, bound to "activeInit" would override steeringVector using this new global. Something like:

SET steeringOverride TO steeringVector. // copy the last known value
LOCK STEERING TO steeringOverride. // steal control

This would sort out all the problems with IPU and whatever.

Furthermore, the addon itself could read the transform from vehicle info: IF controls:HASKEY("steeringTransform") and load the function or disable itself. You could even supply a "default" delegate in the code and use that if controls["steeringTransform"] is some special value, maybe "default" or whatever you like.

Thoughts?