CC: Tweaked

CC: Tweaked

42M Downloads

Gps.locate uses stale ping responses from older pings if they are still in event queue.

Wojbie opened this issue ยท 10 comments

commented

Minecraft Version

1.16.x

Version

1.98.1

Details

When running gps.locate() function multiple times in row one can accidentally cause it to give out incorrect coordinates due to it using older ping responses that were still in event queue. This happens due to fact that it will use first set of pings that give it valid coordinates and leftover pings form any extra gps hosts will stay in queue to be picked by another call.

Easiest way to visualize the bug is to run this code on pocket:

local a,b,c
while true do
 local x,y,z = gps.locate()
 if x ~= a or y ~= b or z ~= c then
  print(x,y,z)
  a,b,c = x,y,z
 end
end

Moving straight up generates this output.
obraz

Possible solutions

Issue can be partially negated by adding code that clears out current events from queue example:

local a,b,c
while true do
 os.queueEvent("gpstest")
 os.pullEvent("gpstest")
 local x,y,z = gps.locate()
 if x ~= a or y ~= b or z ~= c then
  print(x,y,z)
  a,b,c = x,y,z
 end
end

But that don't resolve the issue as there is always possibility that there are still gps hosts that have not yet responded to old ping.

This code resolves issue in single run cases by waiting 0.05 s before sending first ping...

local a,b,c
while true do
 sleep(0.05)
 local x,y,z = gps.locate()
 if x ~= a or y ~= b or z ~= c then
  print(x,y,z)
  a,b,c = x,y,z
 end
end

But i am not sure if it is a proper solution as it feels hacky and I believe (but was unable to test) that multiple gps.locate() calls ran in parallel may still use stale data created by other one if timing happens to be bad...

I spend some time thinking if there is a backwards compatible way to update ping protocol to have unique ids for each ping to ensure that any locate will only listen to its own ping responses but sadly i have not found totally backwards compatible way..
One of solutions would be to move to a different gps protocol that sends some kind of PING table with unique id inside instead of "PING" string and have hosts still respond to both "PING" strings as well as new pings with tables of { x, y, z, ["id"]=pingID }.

Postscriptum

Here is another image showcasing the issue using plethora lines:
obraz
And after adding sleeps:
obraz

commented

But that don't resolve the issue as there is always possibility that there are still gps hosts that have not yet responded to old ping.

Surely this bit isn't an issue as if they haven't sent the message yet then the distance parameter will be correct? I don't know - haven't tested!

commented

But that don't resolve the issue as there is always possibility that there are still gps hosts that have not yet responded to old ping.

Surely this bit isn't an issue as if they haven't sent the message yet then the distance parameter will be correct? I don't know - haven't tested!

When i ran test for that version of code i had way less wrong offsets but they still happened.
obraz

We may be hitting those lovely timing issues.. that's why i was thinking about unique ping id system to sidestep timing.

commented

I also want to add something to this.

Can it also be possible to force gps module to wait more then 4 answers to avoid overconfiguration problem?

Currently, if 4 closest computers will not form correct gps group, response will be failed or incorrect, even if, for example, 8 closest computers forming a corrrect gps group

commented

Currently, if 4 closest computers will not form correct gps group, response will be failed or incorrect, even if, for example, 8 closest computers forming a corrrect gps group

I don't believe that is what actually happens.

if tFix.nDistance == 0 then
	pos1, pos2 = tFix.vPosition, nil
else
	table.insert(tFixes, tFix)
	if #tFixes >= 3 then
		if not pos1 then
			pos1, pos2 = trilaterate(tFixes[1], tFixes[2], tFixes[#tFixes])
		else
			pos1, pos2 = narrow(pos1, pos2, tFixes[#tFixes])
		end
	end
end
if pos1 and not pos2 then
	break
end

Looking at the code it will get ambiguous coords with first 3 and then try to narrow to one of them with next packets it sees. Or until it times out. There is nothing in code that makes it stop after first 4 pings.

Could you provide example of your issue so it can be debugged?

commented

I spend some more time testing this issue and it seems that adding just simple sleep(0.05) at beginning of gps.locate() function code to make it run from clear-ish state does make it work well even in parallel case i was afraid of. So unless someone feels like we really need to make new gps protocol it would be indeed a solution?

commented

I spend some more time testing this issue and it seems that adding just simple sleep(0.05) at beginning of gps.locate() function code to make it run from clear-ish state does make it work well even in parallel case i was afraid of. So unless someone feels like we really need to make new gps protocol it would be indeed a solution?

This sounds like a queue-clearing strategy I like to use where I'll randomly generate an event name, os.queueEvent it, and then steamroll over all events until I pull my random event from the queue (lines 98-99 here). The benefit of this is that it's completely synchronous as long as the queue is behaving as expected (i.e. no misbehaving emulator or HLOS).

The downside is that downstream programs do not generally expect API functions that they call to steamroll the event queue except in certain cases, but this is one of those cases.

commented

I just had a rather random idea here. Rather than having something which flushes the queue, have the GPS broadcaster also broadcast the computer ID. This way, if the receiver receives multiple gps responses from the same computer, it can know to discard the first one (oldest) and keep the newest one.

It will not resolve all issues, but in theory it could at least help mitigate it.

Update: I'm told that CC computers will always run in the same order so it would not help in any cases. Sad.

commented

I don't believe that would help. Look at examples. This is single computer making multiple requests quickly. Having unique id for each request would solve things but would not be backwards compatible and would meant we have to run 2 pararell systems.

commented

Would breaking backwards compat here be all that bad? I don't think I've heard of anyone shipping their own gps library, rather just using the builtin.

commented

Could we have GPS hosts send two messages? The old message for compatibility and a new one that has expandability in mind.