Orbiter-Forum  

Go Back   Orbiter-Forum > Orbiter Space Flight Simulator > Orbiter SDK
Register Blogs Orbinauts List Social Groups FAQ Projects Mark Forums Read

Orbiter SDK Orbiter software developers post your questions and answers about the SDK, the API interface, LUA, meshing, etc.

Reply
 
Thread Tools
Old 12-27-2016, 09:39 PM   #1
Zatnikitelman
Addon Developer
 
Zatnikitelman's Avatar
Default Multiple calls to SetAttitudeRotLevel interfere?

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:
Code:
//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.
Code:
        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?

Last edited by Zatnikitelman; 12-27-2016 at 11:27 PM.
Zatnikitelman is online now   Reply With Quote
Old 12-28-2016, 07:31 PM   #2
Enjo
Mostly harmless
 
Enjo's Avatar


Default

I do the same in my code and it works perfectly, therefore in your case I'd blame Undefined Behavior.
Enjo is offline   Reply With Quote
Old 12-28-2016, 08:05 PM   #3
jedidia
shoemaker without legs
 
jedidia's Avatar
Default

Could you describe how exactly it doesn't work? That might allow some conjectures. "Doesn't really work" is somewhat vague.
jedidia is offline   Reply With Quote
Thanked by:
Old 12-29-2016, 01:17 AM   #4
Zatnikitelman
Addon Developer
 
Zatnikitelman's Avatar
Default

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?
Zatnikitelman is online now   Reply With Quote
Old 12-29-2016, 08:10 AM   #5
Enjo
Mostly harmless
 
Enjo's Avatar


Default

Quote:
Originally Posted by Zatnikitelman View Post
 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.
Enjo is offline   Reply With Quote
Old 12-29-2016, 10:43 AM   #6
jedidia
shoemaker without legs
 
jedidia's Avatar
Default

Quote:
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.
jedidia is offline   Reply With Quote
Old 12-30-2016, 03:29 AM   #7
Zatnikitelman
Addon Developer
 
Zatnikitelman's Avatar
Default

No, it's not on github, I'll post the relevant methods here:
Code:
//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.
Zatnikitelman is online now   Reply With Quote
Old 12-30-2016, 06:02 AM   #8
Quick_Nick
Passed the Turing Test
 
Quick_Nick's Avatar
Default

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.

Last edited by Quick_Nick; 12-30-2016 at 06:04 AM.
Quick_Nick is offline   Reply With Quote
Old 12-30-2016, 03:41 PM   #9
Zatnikitelman
Addon Developer
 
Zatnikitelman's Avatar
Default

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.
Zatnikitelman is online now   Reply With Quote
Old 12-30-2016, 06:45 PM   #10
Enjo
Mostly harmless
 
Enjo's Avatar


Default

I frown at this:
PHP Code:
return (this->*engthrt[model-1])(time); 
If you at least used the STL vector's at(), you'd at least eliminate one suspect.
Enjo is offline   Reply With Quote
Old 12-30-2016, 09:20 PM   #11
jedidia
shoemaker without legs
 
jedidia's Avatar
Default

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

Code:
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:
Quote:
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...

Last edited by jedidia; 12-30-2016 at 09:37 PM.
jedidia is offline   Reply With Quote
Old 12-30-2016, 09:46 PM   #12
Zatnikitelman
Addon Developer
 
Zatnikitelman's Avatar
Default

For clarity sake I eliminated that line and replaced it with:
Code:
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.
Code:
 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:
Code:
bool (Prometheus::*engthrt[4])(const double &);
I do that so I can eliminate the overhead of a conditional:
Code:
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!

Last edited by Zatnikitelman; 12-30-2016 at 09:51 PM.
Zatnikitelman is online now   Reply With Quote
Thanked by:
Old 12-31-2016, 12:16 AM   #13
jedidia
shoemaker without legs
 
jedidia's Avatar
Default

Quote:
The functions actually are members of the Prometheus class.
Wait... they're method pointers?? Now we're getting into really crazy territory...

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.

Quote:
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.

Quote:
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.

Last edited by jedidia; 12-31-2016 at 12:23 AM.
jedidia is offline   Reply With Quote
Old 12-31-2016, 12:25 AM   #14
Zatnikitelman
Addon Developer
 
Zatnikitelman's Avatar
Default

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).
Zatnikitelman is online now   Reply With Quote
Old 12-31-2016, 11:35 AM   #15
jedidia
shoemaker without legs
 
jedidia's Avatar
Default

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:

Code:
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:
Quote:
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:
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:

Code:
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
Code:
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():

Quote:
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:
Quote:
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)?
jedidia is offline   Reply With Quote
Thanked by:
Reply

  Orbiter-Forum > Orbiter Space Flight Simulator > Orbiter SDK


Thread Tools

Posting Rules
BB code is On
Smilies are On
[IMG] code is On
HTML code is Off
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts
Forum Jump


All times are GMT. The time now is 04:44 AM.

Quick Links Need Help?


About Us | Rules & Guidelines | TOS Policy | Privacy Policy

Orbiter-Forum is hosted at Orbithangar.com
Powered by vBulletin® Version 3.8.6
Copyright ©2000 - 2017, Jelsoft Enterprises Ltd.
Copyright 2007 - 2012, Orbiter-Forum.com. All rights reserved.