Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Synchronization settings and UTC time extension #46

Merged
merged 8 commits into from Jul 17, 2017

Conversation

Projects
None yet
2 participants
@raymond30031
Copy link
Contributor

commented Jun 16, 2017

Adds code and example for setting synchronization settings and UTC time

@fcolas
Copy link
Member

left a comment

Many thanks for your efforts, it looks great. There are a few typos and details but it's a great contribution.

Synchronization settings:
The format follows the xsens protocol documentation:
|Function| Line| Trigger Type| Skip First|...
Skip Factor| Pulse Width| Delay or Clock period or offset|

This comment has been minimized.

Copy link
@fcolas

fcolas Jun 16, 2017

Member

Don't put | if you expect coma-separated arguments; actually, you can probably say it explicitly.

The format follows the xsens protocol documentation:
|Function| Line| Trigger Type| Skip First|...
Skip Factor| Pulse Width| Delay or Clock period or offset|
It is required to have all fields presenst in the settings argument

This comment has been minimized.

Copy link
@fcolas

fcolas Jun 16, 2017

Member

presenst -> present; but why not simply: All fields are required.?

4 Interval Transition Measurement
8 SendLatest
9 ClockBiasEstimation
11 StartSampling

This comment has been minimized.

Copy link
@fcolas

fcolas Jun 16, 2017

Member

I had chosen to use two spaces to better single out the value and it's meaning. Conveniently, I had only options with the same width so that meant all lines were aligned (both in order to make these lists easier to read).
I'd propose you align all descriptions by adding a space for lines 1271--1274 and the three other similar blocks below.

This comment has been minimized.

Copy link
@fcolas

fcolas Jun 30, 2017

Member

Ok, I was not clear, the two spaces I was talking about are between the letter/number and its description:
11__StartSampling instead of 11_StartSampling (with space instead of underscore, of course).

@@ -1254,6 +1258,85 @@ def usage():
For longitude, latitude, altitude and orientation (on MTi-G-700):
"pl400fe,pa400fe,oq400fe"
Synchronization settings:
The format follows the xsens protocol documentation:
|Function| Line| Trigger Type| Skip First|...

This comment has been minimized.

Copy link
@fcolas

fcolas Jun 16, 2017

Member

you forgot Polarity

day: day of the month, [1,31]
hour: hour of the day, [0,23]
min: minute of hour, [0,59]
sec: seconds of minute, minute [0,59]

This comment has been minimized.

Copy link
@fcolas

fcolas Jun 16, 2017

Member

Too manyminute.
BTW, the documentation mentions that sec is in 0..59. Did you try to see what happens if you set it to 60 (leap seconds)? Does the firmware report an error and, if not, does it keep the time correctly afterwards?

This comment has been minimized.

Copy link
@raymond30031

raymond30031 Jun 29, 2017

Author Contributor

I tested this. it carries over.

if valid:
return settings
else:
print "Invalide synchronization settings!"

This comment has been minimized.

Copy link
@fcolas

fcolas Jun 16, 2017

Member

Invalide->Invalid

else:
valid = False

if valid:

This comment has been minimized.

Copy link
@fcolas

fcolas Jun 16, 2017

Member

I can see the interest of having a Boolean sometimes but here, I'd prefer if you could merge the two tests.

def get_UTCtime(arg):
# Parse each field from the argument
time_settings = arg.split(',')
time_settings = [int(i) for i in time_settings]

This comment has been minimized.

Copy link
@fcolas

fcolas Jun 16, 2017

Member

Same as above: protect the int call.

if valid:
return time_settings
else:
print "Invalid UTCtime settings!"

This comment has been minimized.

Copy link
@fcolas

fcolas Jun 16, 2017

Member

Not sure the ! is necessary, a full stop is probably good enough.

valid = True
else:
valid = False
if valid:

This comment has been minimized.

Copy link
@fcolas

fcolas Jun 16, 2017

Member

Same as above: use the same test.

Moves UTCtime ns parameter after the second parameter
Fixes typos
Adds integer check for synchronization and UTC time settings
@raymond30031

This comment has been minimized.

Copy link
Contributor Author

commented Jun 29, 2017

Hello Francis,

Sorry for the delay, I have finaly got around to address your comments.
Please review my latest commit.

Cheers,
JC

@@ -577,7 +577,7 @@ def GetUTCTime(self):
struct.unpack('!IHBBBBBB', data)
return (ns, year, month, day, hour, minute, second, flag)

def SetUTCTime(self, ns, year, month, day, hour, minute, second, flag):
def SetUTCTime(self, year, month, day, hour, minute, second, ns, flag):

This comment has been minimized.

Copy link
@fcolas

fcolas Jun 30, 2017

Member

No, don't change internal API, which is intended to follow the documentation and message. The change in order is for the command line interface only.

hour: range [0,23]
min: range [0,59]
sec: range [0,59]
ns: range [0,1000000000]

This comment has been minimized.

Copy link
@fcolas

fcolas Jun 30, 2017

Member

Why did you remove the semantics? I know it's relatively self explanatory, but range [1, 31] is less explanatory than day of the month, [1, 31]

This comment has been minimized.

Copy link
@raymond30031

raymond30031 Jun 30, 2017

Author Contributor

I thought you wanted me to remove it because your previous comment said there are too many minutes

This comment has been minimized.

Copy link
@fcolas

fcolas Jun 30, 2017

Member

Ah, no, it's just that for the seconds, the description had the word minute twice instead of once.

print "Invalid synchronization settings."
return
except:
print "Synchronization settings must be integers."
return

This comment has been minimized.

Copy link
@fcolas

fcolas Jun 30, 2017

Member

The try: except: block should be just around the conversion.
And don't do an empty except: block, you should specify which exception you want to catch, in this case ValueError

print "Invalid UTCtime settings."
return
except:
print "UTCtime settings must be integers."

This comment has been minimized.

Copy link
@fcolas

fcolas Jun 30, 2017

Member

Same comments as above.

@fcolas

This comment has been minimized.

Copy link
Member

commented Jun 30, 2017

Thanks for your work. There are a few small comments but it should be easy to fix.

@raymond30031

This comment has been minimized.

Copy link
Contributor Author

commented Jul 5, 2017

Hello Francis,

I addressed your comments and extended the functionality for UTCtime settings.
We can now either set the current time or a specified time to be the UTCtime for the MTi.

Cheers,
JC

# If argument is now, fill the time settings with the current time
# else fill the time settings with the specified time
if arg == "now":
timestamp2 = datetime.datetime.utcnow() # use datetime to get microsecond

This comment has been minimized.

Copy link
@fcolas

fcolas Jul 7, 2017

Member

That should be timestamp instead of timestamp2!

This comment has been minimized.

Copy link
@raymond30031

raymond30031 Jul 9, 2017

Author Contributor

oops

SetUTCTime settings:
There are two ways to set the UTCtime for the MTi.
Option #1: set MTi to the current UTC time based on local system time

This comment has been minimized.

Copy link
@fcolas

fcolas Jul 7, 2017

Member

It's really a great idea!
But you should say how to use this option (providing keyword 'now') in the usage description and not just in the example.

This comment has been minimized.

Copy link
@raymond30031

raymond30031 Jul 11, 2017

Author Contributor

addressed

# check synchronization settings
if settings[0] in (3, 4, 8, 9, 11) and \
settings[1] in (0, 1, 2, 4, 5, 6) and \
settings[2] in (1, 2, 3) and \

This comment has been minimized.

Copy link
@fcolas

fcolas Jul 7, 2017

Member

settings[2] can be also be 0; that's actually the only way the clear the synchronization settings

This comment has been minimized.

Copy link
@raymond30031

raymond30031 Jul 11, 2017

Author Contributor

I see what you mean now

This comment has been minimized.

Copy link
@raymond30031

raymond30031 Jul 11, 2017

Author Contributor

added

@@ -1370,6 +1459,16 @@ def main():
print "xkf-scenario argument must be integer."
return 1
actions.append('xkf-scenario')
elif o in ('-y', '--synchronization'):
sync_settings.append(get_synchronization_settings(a))
if sync_settings is None:

This comment has been minimized.

Copy link
@fcolas

fcolas Jul 7, 2017

Member

Argh. Doesn't work since your testing a list and not the last element: it should be something like:

new_sync_setting = get_synchronization_settings(a)
if new_sync_setting is None:
    return 1
sync_settings.append(new_sync_setting)

This comment has been minimized.

Copy link
@raymond30031

raymond30031 Jul 11, 2017

Author Contributor

good catch, addressed

Note: The entire synchronization buffer is wiped every time a new one
is set, so it is necessary to specify the settings of multiple
lines at once.

This comment has been minimized.

Copy link
@fcolas

fcolas Jul 7, 2017

Member

You should also say how to clear the synchronization settings. Better yet, why not a keyword clear as in SetUTCTime? (which would send a single sync setting with polarity 0.)

This comment has been minimized.

Copy link
@raymond30031

raymond30031 Jul 11, 2017

Author Contributor

Is the plan to create a function that clears all synchronization settings?

This comment has been minimized.

Copy link
@raymond30031

raymond30031 Jul 11, 2017

Author Contributor

added

@fcolas

This comment has been minimized.

Copy link
Member

commented Jul 7, 2017

Dear JC,

Thanks again for your efforts. Please fix the typo for the time and the synchronization settings and I'll merge it in.

Best,

@raymond30031

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2017

Hello Francis,

I have encountered a weird problem with my MTi G 710, and it does not happen to my MTi 300 or MTi 30.
Step 1:
Setting UTCtime to now and verifies using ./mtdevice -i to check the time is in fact the current time (my computer time)
command:
./mtdevice.py -c iu200,oq200fe,aa200fe,wr200fe,sw200 -u now

Step 2:
roslaunch the xsens_driver launch file and check the time in the imu_data_str

The problem is that the time in the imu_data_str becomes sometime in 2013 and this is repeatable.
The flags is 240 in the time part of the message and the status word is 131 (which means no error)

Do you have any idea why this is happening?

@raymond30031

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2017

Another odd thing I noticed today is that even though we assign iu and sw to be 200 and it does output at 200 Hz, but the output config from inspection shows that these two fields have the value of 65535 (maxed out for int?)

raymond30031 added some commits Jul 11, 2017

Adds argument to clear the synchronization buffer
Renames settings in get_synchronization_settings function to better follow convention
@fcolas

fcolas approved these changes Jul 17, 2017

@fcolas fcolas merged commit a8e7df5 into ethz-asl:master Jul 17, 2017

@fcolas

This comment has been minimized.

Copy link
Member

commented Jul 17, 2017

Many thanks for your work: it is merged in.

@raymond30031

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2017

Glad I can contribute.
@fcolas
Did you get a chance to test -u now with a MTi-G-710 and see if the time gets reset upon startup?

@fcolas

This comment has been minimized.

Copy link
Member

commented Jul 18, 2017

Oops, I had missed this.
I wonder if it shouldn't be in a separate issue.
In the documentation, only the last 3 bits are meaningful with 0x04 stating whether it is fully valid or not. In your case it is 0 so it's not inconsistent. My hypothesis would be that it is searching for the GPS and outputting some initial value until it gets a fix. I agree it shouldn't be the case (with the iu option rather than gu), but I believe it's a firmware issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.