SDK Question Multiple calls to SetAttitudeRotLevel interfere?

[Zatnikitelman]

I've noticed some odd behavior with the above method. It appears when I have two calls to this method, but for two different axes, the latter call doesn't seem to work. So my scenario looks like this:

//In clbkPreStep
sprintf(oapiDebugString(), "Roll %lf %lf %lf", rollval, dCurrRoll, tgt-dCurrRoll);
SetAttitudeRotLevel(2, rollval);
//...
SetAttitudeRotLevel(0, pitchval);

When the second call is commented out, the first call does exactly as it's supposed to. When they're both "active" however, the first call is ineffective. As you can see, I output a debug string which shows the commanded value and the correct rollva is being passed in both scenarios.

Is there a good workaround for this? Only alternative I know is to have an if statement and set the value of THGROUP_ATT_BANKLEFT and THGROUP_ATT_BANKRIGHT depending on the sign of rollval. But that feels messy to me.

Thanks!

[EDIT] Nevermind, doing my suggested workaround doesn't work either, so does the SetAttitudeRotLevel interfere with any call to any thruster group setting?

[EDIT2] There's just something weird about Orbiter I think. I converted both my pitch and roll controllers to set the thruster groups directly, and it's STILL showing the same behavior.

        if (pitchval > 0)
		this->SetThrusterGroupLevel(THGROUP_ATT_PITCHUP, pitchval);
	else
		this->SetThrusterGroupLevel(THGROUP_ATT_PITCHDOWN, pitchval);

Roll is setup the same way with the values changed appropriately. I still get the same result. If I comment out one or the other, the individual controller does exactly what it's supposed to. But when they're both working together, neither really work well. What is going on here?

 

[Enjo]

I do the same in my code and it works perfectly, therefore in your case I'd blame Undefined Behavior.

 

[jedidia]

Could you describe how exactly it doesn't work? That might allow some conjectures. "Doesn't really work" is somewhat vague.

 

[Zatnikitelman]

When I wrote that, it was simply not working. As in there was no roll control at all.

Now, somehow, and I don't know how, it only works from external view. If I go into the glass cockpit (it's a launcher, no real cockpit), it goes out of control when it gets to that phase of the launch. If I watch the whole thing in external view, it flies exactly as intended. Anyone have any ideas here?

 

[Enjo]

Now, somehow, and I don't know how, it only works from external view. (...) Anyone have any ideas here?

Undefined Behavior. Somewhere you're overwriting a memory address which you shouldn't.

 

[jedidia]

Undefined Behavior.

If he were using his own panel, I would agree with that. However, I can't quite see what of his code would be running in the glass cockpit that doesn't also run in external view... It's still a possibility, but it makes me somewhat hesitant.

Zatnikitelman, I'm afraid we're going to need more code in order to be of some actual help. Trouble is, I can't exactly tell you the scope of the code it might be hiding in, since right now there's no way of telling where exactly it originates. Is this code on Github by any chance?
Otherwise, the complete methods dealing with the attitude controls would certainly be a good start.

 

[Zatnikitelman]

No, it's not on github, I'll post the relevant methods here:

//This method is just called from clbkPreStep
bool Prometheus::updateGuidance(const double &time)
{
	//mode 0 is engines only
	if(guidanceRunning)
	{
		//throttle engine user group
		if(guidanceMode == 0)
		{
			return (this->*engthrt[model-1])(time);
		}
		else if(guidanceMode == 1)
		{
			//This is where things get tricky
			//Maintain vertical flight till 200m altitude
			//Begin roll program
			//Then begin pitch routine on roll conclusion
			if (phase == 0)
			{
				if (GetAltitude() >= 200)
				{
					phase = 1;
				}
			}
			else if (phase == 1)
			{
				if (rollGuidance(time))
				{
					phase = 2;
				}
			}
			else if (phase == 2)
			{
				if (GetPitch()*DEG > 88)
				{
					SetThrusterGroupLevel(THGROUP_ATT_PITCHUP, 1);
				}
				else
				{
					phase = 3;
				}
			}
			else if (phase == 3)
			{
				rollGuidance(time, 180);
				if (pitchGuidance(time))
				{
					phase = 4;
				}
			}
			else if (phase == 4)
			{
				if (apoapsisGuidance(time))
				{
					phase = 5;
				}
			}
			return (this->*engthrt[model - 1])(time);
		}
	}
	return false;
}

//Here are the roll and pitch guidance methods
bool Prometheus::pitchGuidance(const double &time)
{
	double pitchtgt = (-0.0014*GetAltitude())+91;
	double pitchval = pids[0]->update(pitchtgt,GetPitch()*DEG,time);
	if (pitchval > 0)
		this->SetThrusterGroupLevel(THGROUP_ATT_PITCHUP, pitchval);
	else
		this->SetThrusterGroupLevel(THGROUP_ATT_PITCHDOWN, -pitchval);
	sprintf(oapiDebugString(), "Pitch %lf %lf %lf", GetPitch()*DEG, pitchtgt, pitchval);
	//this->SetAttitudeRotLevel(0, pitchval);
	if (GetAltitude() >= 50000)
		return true;
	return false;
}
bool Prometheus::rollGuidance(const double &time)
{
	double headingtgt = 90;

	VECTOR3 v;
	HorizonRot(_V(0, 1, 0), v);
	double angle = ((PI + atan2(v.x, v.z))*DEG) - 180;
	/*if (angle <= 0)
		angle += 360;*/

	PIDData p;
	p.actual = angle;
	p.time = time;
	rollpidlist.push_back(p);
	double rollval = pids[1]->update(headingtgt,angle,time);
	this->SetAttitudeRotLevel(2, rollval);
	if (abs(angle - headingtgt) <= 0.1 )
		return true;
	return false;
}

bool Prometheus::rollGuidance(const double &time, double tgt)
{
	double dCurrRoll = GetBank()*DEG;
	if (dCurrRoll < 0)
	{
		dCurrRoll += 360;
	}
	double rollval = pids[1]->update(tgt, dCurrRoll, time);
	//sprintf(oapiDebugString(), "Roll %lf %lf %lf", rollval, dCurrRoll, tgt - dCurrRoll);
	if (rollval < 0)
		this->SetThrusterGroupLevel(THGROUP_ATT_BANKLEFT, -rollval);
	else
		this->SetThrusterGroupLevel(THGROUP_ATT_BANKRIGHT, rollval);
	//this->SetAttitudeRotLevel(2, rollval);
	return false;
}

That's about all I have, I haven't worked on this since my last post. I've checked the framerates, but they're pretty consistent between internal and external.

 

[Quick_Nick]

The rollGuidance method overload that you actually call seems to use the roll thruster group but tries to maintain a specific yaw/heading. What's up with that?

Unrelated nitpick: ((PI + atan2(v.x, v.z))*DEG) - 180 is the same as atan2(v.x, v.z)*DEG.

 

[Zatnikitelman]

The first RollGuidance method is used when the vehicle is vertical. So it rolls to a point where the top of the vehicle (+Y Axis) points to the desired heading. Then when the pitch program engages, it pitches "up" which puts the vehicle in a heads-down orientation.

 

[Enjo]

I frown at this:

return (this->*engthrt[model-1])(time);

If you at least used the STL vector's at(), you'd at least eliminate one suspect.

 

[jedidia]

Well, this is about the same line as Enjo took an issue with, but I'll repost it anyways:

return (this->*engthrt[model-1])(time);

I must honestly say that I'm not 100% sure what you're doing here. It looks like you are invoking a function pointer stored in an array... while possible, that would be highly irregular in C++. It would fit in functional JavaScript, Clojure or possibly functional C (outch!), but in an object-oriented structure this way of doing things can get you into trouble very, very quickly, and your encapsulation sucks by default (as the functions pointed to won't be members of any classes, which you should avoid as much as possible).

In an OO-structure, the way to go about such a problem would be to make an abstract class, implementing the different operations in child classes and then storing instances of these classes in the array.

But as mentioned, I'm not 100% sure that's what you're doing.

Also, stuff like this seems creepy:

bool Prometheus::rollGuidance(const double &time)

Why pass time as const reference exactly? If time must not be modified, then better don't pass it as a reference at all, so you are guaranteed that it won't be affected by the called method in the first place.

Also, could you post the definition of PIDData? There's a potential problem there that could lead to undefined behavior in connection with time being a reference...

 

[Zatnikitelman]

For clarity sake I eliminated that line and replaced it with:

return engineGuidance(time);

But I still have the same results.

PIDData is something for testing, I was using it to help determine how fast my PID Loop settled and how much it overshot so I could dial in the gain values. It's only ever used to hold data, then print data up in clbkSaveState.

 struct PIDData
	 {
		 double time;
		 double actual;
	 };

Jedidia is closest to guessing what that weird line does. The functions actually are members of the Prometheus class. This is how the engthrt array is defined:

bool (Prometheus::*engthrt[4])(const double &);

I do that so I can eliminate the overhead of a conditional:

if(model == 1)
  //throttling logic for booster type 1
else if(model == 2)
  //throttling logic for booster type 2
else if(model == 3)
 //throttling logic for booster type 3
else if(model == 4)
 //throttling logic for booster type 4

Is the overhead small? Yes. But I don't know how many rockets the user will want. For all I know, someone will try to launch a constellation of 100 Prometheuses at once. I'm more than a bit OCD on code efficiency. Really this specific line is...not the greatest, I need to refactor the engineGuidance method itself to take into account multiple models, at the time I originally wrote this, I was assuming four different throttling profiles, but after testing, I only ended up with two.

As for const double &time. I believed at the time it was faster, now (4 years later) I'm not so sure.

Again, thanks everyone for at least engaging me on this. Regardless of a final verdict, I do appreciate the time you all are putting toward helping me! :thumbup:

 

[jedidia]

The functions actually are members of the Prometheus class.

Wait... they're method pointers?? Now we're getting into really crazy territory... :blink:

I do get why one would use method pointers for an event engine, but even for that there's better solutions in Object-oriented programming. And as far as I can see, this is just simple logic.

For all I know, someone will try to launch a constellation of 100 Prometheuses at once.

But... but... each of them would be its own instance of the prometheus class, so it wouldn't matter. Do you do any state sharing between these instances? because then we'd get into territory where any kind of weird behavior becomes very likely indeed.

As for const double &time. I believed at the time it was faster, now (4 years later) I'm not so sure.

It still is (at least in a 32-bit build, which orbiter is), but what you gain from copying 32 bits (size of the reference) vs. 64 bits (size of a double) is really insignificant when considering the additional complexity and error-pronenes of the code.

 

[Zatnikitelman]

No, there shouldn't be any sharing of state. The only things I would share is anything that would only be read and not written to (such as constants for defining mesh, thruster, fairing offsets).

 

[jedidia]

Ok, I think I need to understand the architecture a bit better before I can go about looking for bugs in there. You're certeinly doing something rather unorthodox there, and it brings a huge potential for weird behavior with it.

For example, when you write this:

if(model == 1)
  //throttling logic for booster type 1
else if(model == 2)
  //throttling logic for booster type 2
else if(model == 3)
 //throttling logic for booster type 3
else if(model == 4)
 //throttling logic for booster type 4

What exactly is a "booster type"? If this is the prometheus from the movie prometheus, I can't remember the vessel having any detachable boosters or anything, so either I'm thinking about a completely different thing, or this is not what you mean. Could you explain what a "booster type" is and what it does?

And then, there's still this remark:

But I don't know how many rockets the user will want. For all I know, someone will try to launch a constellation of 100 Prometheuses at once.

Which seems to be related to those booster types, but I can't think of a way how that matters in any way. If there's no static states in the prometheus class, then it shouldn't matter at all if there are multiple instances of it around, ergo I still don't see how you get from "There might be multiple instances" to "I'll call perfectly normal methods via pointer". Since this is currently the thing that strikes me as oddest about the code, and also the thing I understand the least about it, that would be where I'd start looking for the bug. Doesn't mean it has to be there though, but before I understand what's going on here there's not much point in looking elsewhere.

Posting the header file of the class might help me a bit connecting the dots.

---------- Post added at 10:47 AM ---------- Previous post was at 10:35 AM ----------

Sorry, Obviously I was thinking about something completely different, since I just took a look at your add-on list: [ame="http://www.orbithangar.com/searchid.php?ID=5506"]PrometheusLV For Orbiter10[/ame]

So, I assume "booster types" are different types of boosters that get attached to the central rocket. Now here's the propper way you would do this in an OO-way:

class booster_base
|- class booster_type_one
|- class booster_type_two
|- class booster_type_three
|-etc.

Have an array or a vector of type booster_base in your prometheus class, where all the attached boosters are contained.

Have a loop doing something like

for (UINT i = 0; i < boosters.size(); ++i)
{
    boosters[i].do_stuff(time);
}

This way you can handle states of every booster individually in their instance, childclasses can just override the do_stuff method and implement their own behavior, and you end up with code that has a lot less possibility of doing things you never wanted it to do.

---------- Post added at 12:35 PM ---------- Previous post was at 10:47 AM ----------

Something a bit closer to the actual problem you're having and a bit less about the weird structure. Have you considered this property of SetAttitudeRotLevel():

Calling this method can have side effects if the RCS thrusters for the specified axis are also registered for other attitude axes.

In other words, you're setting the levels for the roll groups and the pitch groups:

SetAttitudeRotLevel(2, rollval);
//...
SetAttitudeRotLevel(0, pitchval);

But if some thrusters are part of both the roll groups and the pitch groups, the second call will indeed overwrite their prior setting, a behavior that would exactly match your problem. So, is every one of your RCS thrusters only in one thruster group, or do some of them pull double duty (which wouldn't be out of the ordinary)?

 

[martins]

When commenting on the behaviour of SetAttitudeRotLevel, could you also state if you are using the 2016 release version or the latest beta? This function has changed since the release.

 

[Zatnikitelman]

*SNIP*

And then, there's still this remark:


Which seems to be related to those booster types, but I can't think of a way how that matters in any way. If there's no static states in the prometheus class, then it shouldn't matter at all if there are multiple instances of it around, ergo I still don't see how you get from "There might be multiple instances" to "I'll call perfectly normal methods via pointer". Since this is currently the thing that strikes me as oddest about the code, and also the thing I understand the least about it, that would be where I'd start looking for the bug. Doesn't mean it has to be there though, but before I understand what's going on here there's not much point in looking elsewhere.

Posting the header file of the class might help me a bit connecting the dots.

Since you did realize what was being talked about booster-wise, I'll leave that out. But in terms of the sentence about multiple instances. What I mean is, that's why I'm so performance-conscious. No, they don't share state, but I still want to improve the efficiency of anything that's called every frame as much as I possibly can. It's been a few years (and a different computer) since I measured it, but some of the "strange" ways I improve performance in Prometheus did have a demonstrable impact on framerate even with just one vehicle instance present.

Sorry, Obviously I was thinking about something completely different, since I just took a look at your add-on list: PrometheusLV For Orbiter10

So, I assume "booster types" are different types of boosters that get attached to the central rocket. Now here's the propper way you would do this in an OO-way:

class booster_base
|- class booster_type_one
|- class booster_type_two
|- class booster_type_three
|-etc.

Have an array or a vector of type booster_base in your prometheus class, where all the attached boosters are contained.

Have a loop doing something like

for (UINT i = 0; i < boosters.size(); ++i)
{
    boosters[i].do_stuff(time);
}

This way you can handle states of every booster individually in their instance, childclasses can just override the do_stuff method and implement their own behavior, and you end up with code that has a lot less possibility of doing things you never wanted it to do.

I don't inherently disagree, but it does feel like extra effort and overhead for little to no benefit. I don't see a performance improvement coming from that.

Something a bit closer to the actual problem you're having and a bit less about the weird structure. Have you considered this property of SetAttitudeRotLevel():



In other words, you're setting the levels for the roll groups and the pitch groups:


But if some thrusters are part of both the roll groups and the pitch groups, the second call will indeed overwrite their prior setting, a behavior that would exactly match your problem. So, is every one of your RCS thrusters only in one thruster group, or do some of them pull double duty (which wouldn't be out of the ordinary)?

Yep, that would do it before I changed out SetAttitudeRotLevel for direct calls to SetThrusterGroupLevel

When commenting on the behaviour of SetAttitudeRotLevel, could you also state if you are using the 2016 release version or the latest beta? This function has changed since the release.

This is still 2010. Though I'll convert to 2016 not long after release (or whatever the current Orbiter is by then at the rate I'm going) I want to stabilize everything I can before introducing new variables.

 

[Enjo]

Have an array or a vector of type booster_base

Obviously you meant std::vector<booster_base *>, or better yet: std::vector<std::unique_ptr<booster_base>>... phew.

I don't inherently disagree, but it does feel like extra effort and overhead for little to no benefit. I don't see a performance improvement coming from that.

There's no performance improvement from the strategy pattern. It's used for better maintenance.

 

[jedidia]

I don't see a performance improvement coming from that.

There isn't. At least not if you consider performance purely as "performance of the code when executed". If you take into account "performance of the programmer that has to maintain and debug the code", there's quite some improvement without actual loss of computing performance. The memory overhead is larger, yes, but we've stoped worrying about that since the time we first measured RAM sizes in Gigabytes...

Anyways, if you're confident that the bug is not located in that part of the code, which it well might not, I'll stop pressing the issue. Your way does work, it's just very difficult to read and hard to maintain and debug, both things that are very important in todays coding paradigms, and the reason why object oriented programming was invented in the first place.

Obviously you meant std::vector<booster_base *>, or better yet: std::vector<std::unique_ptr<booster_base>>... phew.

Yes, the terminology I chose was wrong. I'm not sure why, but I have this thing where I think of "iterables" (pardon my Java) as "being of type X" instead of "containing objects of type X", even though I'm fully aware of the difference.

Yep, that would do it before I changed out SetAttitudeRotLevel for direct calls to SetThrusterGroupLevel

If you have thrusters in multiple groups, you still have the same problem, even if you use SetThrusterGroupLevel(). In fact, I would assume that SetAttitudeRotLevel() is merely a wrapper that calls SetThrusterGroupLevel(), or at least applies the same operations to the groups as the other method. It's merely got a more convenient interface.

 

[Zatnikitelman]

*SNIP*

If you have thrusters in multiple groups, you still have the same problem, even if you use SetThrusterGroupLevel(). In fact, I would assume that SetAttitudeRotLevel() is merely a wrapper that calls SetThrusterGroupLevel(), or at least applies the same operations to the groups as the other method. It's merely got a more convenient interface.

:facepalm: I should have seen this coming. Ok, so, I'm making progress. I added independent roll thrusters. It seems to have pretty much solved it. The only sticking point is sometimes depending on how the vehicle rolls during ascent, it may end up way off heading. My target heading during testing has always been a hard-coded 90 degrees, but sometimes I end up at something like 40 degrees, or even 327. I think it's just a matter of tuning all the parts of the autopilot, including literally tuning the PID loop that controls the vertical roll.

Again, thanks so much for sticking with me through all of this and dealing with my terrible, old, code! :thumbup: