JSBSim-Team / jsbsim

An open source flight dynamics & control software library
GNU Lesser General Public License v2.1
1.38k stars 454 forks source link

Feature request: Per-gear height of terrain #1138

Open vranki opened 2 months ago

vranki commented 2 months ago

I'm submitting a ...

Describe the issue

I'm using a CIGI based image generator which can be queried for terrain elevation at specific points.

One can create custom ground callback implementations in JSBSim, but that approach doesn't really work when using CIGI which requires you to query the IG over network.

My request would be to allow setting terrain elevation for each FGLGear instance. That way user could setup CIGI Height of Terrain requests for each gear and use the provided value for each FGLGear.

Currently it is possible to work around this by calculating ground normal below aircraft and calculating elevations for each gear position relative to aircraft center with vector maths, but this is quite complex and doesn't work well in all situations.

seanmcleod commented 2 months ago

I'm confused. You mention having to perform a network call and that being an issue, but then you talk about doing a CIGI network call for each gear and that isn't an issue now?

vranki commented 2 months ago

The difference is that you can "subscribe" to HoT updates from CIGI and it sends the updates for each frame asynchronously. With callback method you need to send a request, wait for next frame and handle the response in the callback itself. That's not an option. In my suggestion the code that handles update from CIGI (really a UDP packet) could set the HoTs for each gear before running a flight loop iteration.

seanmcleod commented 2 months ago

Okay, that's a bit clearer now. Although I still have a couple more questions 😉

What rate is JSBSim running at, the default 120Hz? What rate is the CIGI sending it's async HoT updates? How out of sync can the HoT message be relative to JSBSim's current position? Given the update frequency of the CIGI's position by JSBSim and the round-trip delay concluding with the arrival the async UDP packet.

Is the HoT message sending a single terrain elevation value for the CIGI's current position (which may be slightly out of sync) or does it reply with a mini-DEM around the CIGI's current position?

What sort of DEM horizontal resolution are you dealing with? Is it fine enough to have different elevation values given the aircraft's size for the different gears and airframe contact points?

You might also find some of the discussion from the recent pull request I submitted - https://github.com/JSBSim-Team/jsbsim/pull/1104 useful.

Actually, re-reading your original message:

user could setup CIGI Height of Terrain requests for each gear

Is this some sort of once-off setup request you send to the CIGI, somehow detailing the relative position of each gear and other structural contact points to the reference eye-point that the CIGI uses to generate the visuals? And then the CIGI receives updates to the aircraft's pose and using the pose and the relative positions of each gear it can use their individual positions to perform a DEM lookup/raycast and return that for each gear etc.?

vranki commented 2 months ago

What rate is JSBSim running at, the default 120Hz? What rate is the CIGI sending it's async HoT updates? How out of sync can the HoT message be relative to JSBSim's current position? Given the update frequency of the CIGI's position by JSBSim and the round-trip delay concluding with the arrival the async UDP packet.

I'm running flight model currently at 480Hz but this can be configured. CIGI runs at fixed 60Hz. I understand HoT is not up to date for every simulation cycle as the aircraft moves, but at 60Hz it's accurate enough not to be noticeable.

Is the HoT message sending a single terrain elevation value for the CIGI's current position (which may be slightly out of sync) or does it reply with a mini-DEM around the CIGI's current position?

It can be configured to send HoT's at points relative to ownship aircraft (usually landing gears). No mini-DEM.

What sort of DEM horizontal resolution are you dealing with? Is it fine enough to have different elevation values given the aircraft's size for the different gears and airframe contact points?

Runways can be sloped and if you go outside runway there can be quite steep gradients in the ground data.

You might also find some of the discussion from the recent pull request I submitted - #1104 useful.

Thanks, I'll read that.

Is this some sort of once-off setup request you send to the CIGI, somehow detailing the relative position of each gear and other structural contact points to the reference eye-point that the CIGI uses to generate the visuals? And then the CIGI receives updates to the aircraft's pose and using the pose and the relative positions of each gear it can use their individual positions to perform a DEM lookup/raycast and return that for each gear etc.?

Correct, That's how it works. You can read more details at CIGI 3.3 spec at https://cigi.sourceforge.io/specification.php sections 4.1.24 HAT/HOT Request and 4.2.2 HAT/HOT Response

seanmcleod commented 2 months ago

Out of interest, why 480Hz?

Thanks for the CIGI spec reference and the specific sections.

So, you setup a HAT/HOT request for each gear specifying their (X,Y,Z) offsets relative to the entity, I'm assuming the CIGI server takes into account the entity's attitude when doing the lookups, and you get to specify a HAT/HOT ID for each one, and an update rate.

So, assuming a 60Hz update rate for the HAT/HOT replies, you receive the (ID, Height) for the various gears which for now you would need to cache in your code. Currently JSBSim would be calling your ground callback at 480Hz, and calling it once per gear passing the ECEF coordinates for the specific gear.

So, at the moment you would need some logic to map the ECEF coordinates supplied to your ground callback to the relevant (ID, Height) tuples you have cached, given you know the mapping between the IDs you assigned and each particular gear.

So, I guess one option is to get the aircraft's cg from JSBSim in the ECEF, and attitude, and do some vector calculations to map to the relevant gear instance? Which will then allow you to return the relevant height from your cached (ID, Height) tuples.

bcoconni commented 2 months ago

I'm struggling to see where the problem lies.

JSBSim is currently calling the ground callback for each gear and the location parameter that is passed to FGGroundCallback::GetAGLevel contains the location of the individual gear that is processed. So JSBSim already does what the PR title is requesting.

https://github.com/JSBSim-Team/jsbsim/blob/0b99bf8b173fe21cd3a1481695ff199b5e4a4335/src/models/FGLGear.cpp#L297-L298 https://github.com/JSBSim-Team/jsbsim/blob/0b99bf8b173fe21cd3a1481695ff199b5e4a4335/src/models/FGInertial.h#L104-L109

Now, if the problem is that you don't want query over the network for each gear independently then I don't see why the CIGI management must be done in the ground callback ?

Assuming you're using C++, you could have some code that runs a step of JSBSim, collects the gears position and sends them to CIGI which could cache the result for later use by the ground callback:

FGFDMExec fdmex;
CIGIManager cigi;
std::vector<FGLocation> gearLoc;

auto gr = fdmex->GetGroundReactions();
auto propag = fdmex->GetPropagate();

while(1) {
  // Run a step
  fdmex->Run();

  // Collect the gears position
  FGLocation CGloc = propag->GetLocation(); // CG location
  gearLoc.clear();
  for (int i=0; i < gr->GetNumGearUnits(); i++) {
    FGColumnVector3 pos = gr->GetGearUnit(i)->GetLocalGear(); // gear positions relative the the CG expressed in the local frame.
    gearLoc.push_back(CGloc.LocalToLocation(pos)); // Convert local positions to ECEF positions and store the result.
  }

  // Run CIGI with the list of all gears position
  cigi.get_data_from_positions(gearLoc);
}

Of course, this is pseudo code. The class CIGIManager is fictitious and std::vector<> might not be the best data structure for CIGI but the idea is that network queries are managed outside the ground callback. Also with the pseudo code above, CIGIManager is queried once and only once per time step and it can cache data for later use by the ground callback.

seanmcleod commented 2 months ago

@bcoconni I don't think the main issue is that network queries are managed outside of the ground callback per se. Rather that while running the JSBSim simulation loop at 480Hz (out of interest, wondering why such a high rate is required) he doesn't want to make any synchronous network calls given that the variable latency of a synchronous network call may add too much jitter to his ideal 480Hz simulation rate.

So instead he wants to process asynchronous ground updates that stream in as UDP packets at 60Hz.

Your sample code has answered my comment with regards to my suggestion:

you would need some logic to map the ECEF coordinates supplied to your ground callback to the relevant (ID, Height) tuples you have cached, given you know the mapping between the IDs you assigned and each particular gear.

So, at startup setup the CIGI request for async streamed ground reports per gear using the gear index as the HAT/HOT ID.

Then when you receive the async UDP packet for the ground elevation for each gear simply store the ground elevation in an array, which maps to the gear index. This is your cache of ground elevations.

Then in your ground callback implementation you're passed the ECEF coordinates of the specific gear that JSBSim wants an answer for as the loc parameter.

double CIGIGroundCallback::GetAGLevel(double t, const FGLocation& loc,
                                    FGLocation& contact, FGColumnVector3& normal,
                                    FGColumnVector3& vel, FGColumnVector3& angularVel) const
{
   // Determine which gear index this loc matches
   FGLocation CGloc = propag->GetLocation(); // CG location
   auto gr = fdmex->GetGroundReactions();
   for (int i=0; i < gr->GetNumGearUnits(); i++) {
      FGColumnVector3 pos = gr->GetGearUnit(i)->GetLocalGear(); // gear positions relative the CG expressed in the local frame.
       if(abs(CGloc.LocalToLocation(pos) - loc) < epsilon)
       {
          // Found matching gear
          ground_height = CIGI_cache[i];
          break;
       }
  }
  ..... rest of ground callback implementation
}
bcoconni commented 2 months ago

@seanmcleod OK thanks for the clarification. The discussion makes sense to me now.

Regarding your implementation of CIGIGroundcallback, you wouldn't need to compare the gears coordinates since the gears are always queried in the same order. And since your PR #1104, it is now guaranteed that each landing gears are queried whether they are retracted or not i.e. no gears are skipped. So all you need is a counter that contains the gear ID:

double CIGIGroundCallback::GetAGLevel(double t, const FGLocation& loc,
                                    FGLocation& contact, FGColumnVector3& normal,
                                    FGColumnVector3& vel, FGColumnVector3& angularVel) const
{
  ground_height = cache[currentID];

  ..... rest of ground callback implementation

  // Increment the gear ID for the next query and make sure that it is reset after
  // all the gears have been queried.
  currentID = (currentID + 1) % nGears;

  return ground_height;
}

and the class CIGIGroundCallback would need the following members:


class CIGIGroundCallback : public FGGroundCallback {
public:
  ...

private:
  unsigned int currentID;
  unsigned int nGears; // number of gears
  CIGI_cache cache;
  ...
};
bcoconni commented 2 months ago

Of course, this implies that CIGI_cache keeps track of the order in which the gears are queried.

seanmcleod commented 2 months ago

the gears are always queried in the same order. And since your PR https://github.com/JSBSim-Team/jsbsim/pull/1104, it is now guaranteed that each landing gears are queried whether they are retracted or not i.e. no gears are skipped. So all you need is a counter that contains the gear ID:

Ah, yes, good point, and does simplify things.

Of course, this implies that CIGI_cache keeps track of the order in which the gears are queried.

I took a look at the CIGI specification document that @vranki linked to, and you as the HOST get to define the HAT/HOT ID for each element you want ground reports on. So my suggestion is to simply use the gear index value from JSBSim as the HAT/HOT ID, which is then included in the response packet.

image

@vranki keep in mind that STRUCTURE contacts defined in the FDM also show up as gear, which also generate a call to the ground callback. gr->GetNumGearUnits() does include these structure contacts.

    <ground_reactions>

        <contact type="STRUCTURE" name="NOSE">
            <location unit="IN">
                <x> 0 </x>
                <y> 0 </y>
                <z> 0 </z>
            </location>
            <static_friction> 0.2 </static_friction>
            <dynamic_friction> 0.2 </dynamic_friction>
            <rolling_friction> 0.2 </rolling_friction>
            <spring_coeff unit="LBS/FT"> 10000 </spring_coeff>
            <damping_coeff unit="LBS/FT/SEC"> 2000 </damping_coeff>
        </contact>  
vranki commented 2 months ago

Thanks for the updates. The fact that gears are queried in order makes things much simpler. I can map CIGI HAT/HOT request to gear/contact number easily. This should be documented so that any future optimizations won't break it.

It might also be a good solution to add the gear index or reference to gear object in the callback function parameters. Then user wouldn't need to maintain a counter of the callbacks.

seanmcleod commented 2 months ago

@vranki out of interest, can you share some info on your setup/project? What CIGI software are you using, what the project is for, why 480Hz etc.?

seanmcleod commented 2 months ago

Hmm, just remembered that there is at least 1 other call to the ground callback that doesn't come from the gear/structure contacts, and that's to calculate the AGL for the aircraft's cg since this AGL is used in other places.

https://github.com/search?q=repo%3AJSBSim-Team%2Fjsbsim+GetContactPoint&type=code

https://github.com/search?q=repo%3AJSBSim-Team%2Fjsbsim+GroundCallback-%3E&type=code

seanmcleod commented 2 months ago

So for now you would have to filter out these calls in terms of incrementing the counter index, by comparing the passed in location with that of the aircraft's cg.

vranki commented 2 months ago

@vranki out of interest, can you share some info on your setup/project? What CIGI software are you using, what the project is for, why 480Hz etc.?

Unfortunately it's a customer project under NDA so I cannot give any details on it. Maybe sometime in future if the project is released. 480Hz is just a guess. With 60Hz flight loop I had some oscillations at transonic speeds. 120Hz would probably be enough but I've understood smaller timestep makes more accurate simulation (up to some point). Anyway it's configurable.

bcoconni commented 2 months ago

Hmm, just remembered that there is at least 1 other call to the ground callback that doesn't come from the gear/structure contacts, and that's to calculate the AGL for the aircraft's cg since this AGL is used in other places.

https://github.com/search?q=repo%3AJSBSim-Team%2Fjsbsim+GetContactPoint&type=code

https://github.com/search?q=repo%3AJSBSim-Team%2Fjsbsim+GroundCallback-%3E&type=code

So for now you would have to filter out these calls in terms of incrementing the counter index, by comparing the passed in location with that of the aircraft's cg.

Ah yes indeed. Good catch !

bcoconni commented 2 months ago

It might also be a good solution to add the gear index or reference to gear object in the callback function parameters. Then user wouldn't need to maintain a counter of the callbacks.

In theory that's feasible. This method would call the legacy method FGGroundCallback::GetAGLevel by default for backward compatibility and could be overridden if the user needs some specific information about the gear.

vranki commented 1 month ago

If I'd like to implement this change to JSBSim, how should I design it? Is backwards compatibility important? Can I just add a extra parameter to the callback with -1 as gear number if not querying a gear? Add another function with the gear number? Or what would be the preferred implementation?

seanmcleod commented 1 month ago

So you've decided not to go with maintaining your counter index? 😉

For this particular case, I'm not sure how much of a deal backwards compatibility is. It would be a minor irritation for a user who pulls the latest JSBSim source and when compiling their app gets a compiler error because there is a new parameter, which they can ignore, they simply need to update their method signature.

  virtual double GetAGLevel(const FGLocation& location, FGLocation& contact,
                            FGColumnVector3& normal, FGColumnVector3& v,
                            FGColumnVector3& w) const

// Changing to

  virtual double GetAGLevel(const FGLocation& location, FGLocation& contact,
                            FGColumnVector3& normal, FGColumnVector3& v,
                            FGColumnVector3& w,
                            int contactIndex=-1) const
bcoconni commented 1 month ago

I'm not sure how much of a deal backwards compatibility is.

The management of the C++ API backward compatibility has historically been quite relaxed. However we should avoid breaking the C++ API when possible (i.e. when the workaround is not too convoluted - which, I admit, leaves a lot of to interpretation :smile:).

Note that the method FGGroundCallback::GetAGLevel(const FGLocation& location, ...) is by default a proxy to the method FGGroundCallback::GetAGLevel(double time, const FGLocation& location, ...) so that it uses JSBSim simulated time.

https://github.com/JSBSim-Team/jsbsim/blob/8e4803b2cbae945fd62e84b1aca121c35edf5174/src/input_output/FGGroundCallback.h#L93-L96

So it's the method FGGroundCallback::GetAGLevel(double time, const FGLocation& location, ...) that should be overloaded unless the application is managing the elapsed time itself.

// Changing to

 virtual double GetAGLevel(const FGLocation& location, FGLocation& contact,
                           FGColumnVector3& normal, FGColumnVector3& v,
                           FGColumnVector3& w,
                           int contactIndex=-1) const

I'd suggest that a const pointer (or a const reference) to the FGLGear instance be passed rather than the contact index. The finality is to get access to FGLGear so this would save writing a couple of lines of code to the developer of the ground interface function.

seanmcleod commented 1 month ago

I'd suggest that a const pointer (or a const reference) to the FGLGear instance be passed rather than the contact index.

Would have to be a const pointer since remember the ground callback is also called by JSBSim to get the AGL of the aircraft's cg, i.e. without a specific gear/contact.

bcoconni commented 1 month ago

Would have to be a const pointer since remember the ground callback is also called by JSBSim to get the AGL of the aircraft's cg, i.e. without a specific gear/contact.

Good point :+1:

bcoconni commented 1 month ago

BTW if we break backward compatibility we could go frankly and redesign the current hideous interface of FGGroundCallback::GetAGLevel (passing arguments by reference so that they get updated is soooo 90's :wink:):

Instead we could have something like:

struct FGContact {
  FGLocation position;
  FGColumnVector3 velocity;
  FGColumnVector3 angularVelocity;
};

FGContact GetAGLevel(const FGLocation& location, const FGLGear*) const;
seanmcleod commented 1 month ago

Talking about the time argument, I've never really understood the reason for it. I guess especially since I assumed the terrain model is generally static and so time independent. Or is this to help for things like modelling dynamic terrain like aircraft carriers? In case the ground callback needs to use the current simulation time to perform a time based calculation or lookup of say the aircraft carrier?

I guess I assumed the terrain/world system would be queried for the current position of objects like aircraft carriers etc. on the assumption that there would be a separate update of world objects for each simulation frame etc.