twisted.positioning -- a better positioning framework

[lvh]

As some of you have undoubtedly noticed, Twisted's current GPS support (twisted.protocols.gps) has issues:

1. The public API sucks:
   1. Fourteen arguments? All of them positional? Really?
   2. fooReceived methods are organized by what happens to be easy for NMEA sentences, not what makes sense
   3. It's hard to extend, raw objects are passed instead of pretty wrappers that abstract technology details away
   4. It's very protocol centric: the API changes between NMEA and Zodiac, a good API would look the same for any positioning technology
2. There are no unit tests.
3. It assumes we're using satellite navigation (well, not really, but only because there's no interface that says what it does)

This ticket is about a proposed alternative, twisted.positioning, which hopefully will:

1. deprecate the existing code
2. support more than satellite-based navigation (such as cell triangulation) or maybe just more than GPS ( Galileo comes to mind)

After reviewing preliminary code and discussing the issue, we came to the following conclusions:

We need to have an interface (tentative name: IPositioningDataReceiver) that implements a number of methods, such as:

1. positionRecieved(self, position)
2. altitudeReceived(self, altitude)
3. headingReceived(self, heading)
4. ...

The parameters of these methods should not be simple objects (like floats or strs or bools), but Position, Heading... objects so that more API can be added later as required.

The classes that implement this should not continuously update their own state. Doing that is the job of the application that uses the framework.

Originally we thought it would be cool if accuracy data came together with positioning data, however this idea was dropped because it was unrealistic with real world positioning devices.

We should try to keep this API as general as possible (making as few assumptions about the underlying protocol as possible).

First thing to do should probably be finalizing the IPositioningReceiver interface. Getting it right is extremely important and has big implementation consequences: NMEA might present data in an illogical or useless order, but this shouldn't change our interface significantly.

This raises problems with the fooReceived methods, since data that is made available in chunks (headingRecieved doing both magnetic and true headings for example) might not have that data made available in the underlying implementation (some NMEA sentences only provide true heading, some proprietary ones only provide magnetic heading, and some sentences provide both -- it doesn't make sense to ignore the data from the first two types of sentence simply because they didn't have both kinds of heading).

The easy way out is to make the objects that the interface deals with as abstract as possible. With enough eyes, every problem is tiny, so your comments are much appreciated as usual.

You can find the code on Github:  http://www.github.com/lvh/twisted has a positioning-3926 branch.

[glyph]

A minor musing: perhaps it would be best to put this new, expanded framework into its own package, 'twisted.position' let's say, to get it a bit more visibility? (It may be best to forestall this discussion until the code's already written, because it's just a distraction, and I think there will be distutils implications; but, I wanted to get the suggestion up here before I forget.)

[lvh]

Sounds like a great idea, but I didn't really think that it was important enough to warrant a toplevel package :-D Also "positioning" isn't nearly a crazy enough name for a top level twisted package. </silly>

Include margin of error information in callbacks, or keep it separate?

I was thinking about what glyph said in earlier discussion about messages that have potential error information, such as:

• positionReceived
• altitudeReceived
• ...

... might want to get that error information there, rather than in a separate callback. After all, you don't care about a position measurement if it has an error that makes it irrelevant to your application.

The counterargument was mainly that, well, NMEA just doesn't want to do that -- there is no single NMEA sentence that is open (in the sense of "not proprietary"), widely supported, and provides you with all data. (For the interested reader: there are no such sentences that are proprietary, either.) For more information on the subject:  http://esr.ibiblio.org/?p=801 (sort of funny, worth a read or quick skim if you're not yet convinced that talking to GPS devices sucks).

However, this feels like I'm mangling the interface just to make it easier for NMEA. This sounds like a bad idea. For example, when you're reading from a gpsd server, you can get all of this information (from now on called TPV+E -- time, position, velocity + errors, which is what you care about 90% of the time, the only thing that's missing is heading/course) at once.

gpsd does this by keeping internal state. We've already decided that keeping internal state would be bad for us, but unfortunately getting meaningful data from an NMEA-spewing GPS appears to be an inherently stateful problem.

So, the question is simple: what's worse? Keeping state, or changing the interface?

The alternative interface which does not keep state has multiple callbacks, like:

• positionReceived(self, latitude, longitude)
• positionErrorReceived(self, horizontalError, verticalError)
• altitudeReceived(self, altitude)
• altitudeErrorReceived(self, altitudeError)

If we kept state, we could do this instead:

• positionReceived(self, latitude, longitude, horizontalError, verticalError)
• altitudeReceived(self, altitudeReceived, altitudeError)

Personally I'm slightly more of a fan of the former because it keeps the NMEA implementation stateless. Then again, I'm stark raving mad.

[lvh]

Alternative interface design

Hi.

This comment is a boiled down version from the ad-hoc code review from last night for which I'd like to really thank the culprits (exarkun, dash, tazle).

The interface as it exists now is pretty dumb, since the class that has lineReceived and fooReceived is the same thing, so either we change how it works or we remove the interface and just add stubs on the subclass.

A proposed solution by tazle involves splitting the current NMEAReceiver class up into two components: the receiver itself, which produces NMEA-centric data in a barely-better-than-raw form (so methods like GPRMCReceived), the interface, and then an NMEA-to-generic-interface adapter that keeps the necessary state to produce data in the useful form.

The only thing I don't like about it is that this is just the same thing as before (keeping state sucks) except it happens in the adapter instead of the receiver. Also, the receiver would be a fairly trivial piece of code: I'm guessing all it would do checksum verification, splitting and then call getattr(self, sentenceType + "Received")(splitSentence). The advantage is that people could do all sorts of gnarly stuff to the raw NMEA data, which I suppose could be useful.

Any comments?

[lvh]

Update on the gpsd thing:

Development on the old protocol handler has halted, esr has said that a new fancy protocol will happen Soon(TM) (time frame: less than a month, more than a week). It's JSON based, and it's much nicer than this thing.

The old protocol won't be going away, but it will be deprecated.

Similarly, "watcher mode" (where you just listen instead of query + wait for response) will become the default. Obviously this is a lot better for an asynchronous framework like Twisted.

[lvh]

Removed for review, since it shouldn't have been in the first place. Uploading a proper patch later.

[lvh]

Hi!

Patch up for review, enjoy.

Comments on the points raised in previous comments on this ticket are still welcome of course :-)

[lvh]

Things that I've seen myself that need reviewing:

• t.p.nmea.NMEAAdapter
  • _cleanCurrentSentence: what would be the best way to guarantee fixer order? I think I can write the fixers in a way that order doesn't matter, but that isn't pretty (at the fixer level). It seems a bit ridiculous to have an entire dependency graph, on the other hand relying on the fact that lexicographical == proper order sucks too, prefixing the fixers with weights sucks. Perhaps an extra dict from fixers to weights or something?

• t.p.nmea.NMEA
  • it seems like it has two ways of getting things to the NMEAAdapter. I'm guessing one of these has to go, but I'm not sure which.
    • the callbacks (nmea_GGA, nmea_RMC...)
    • explicitly, through self.receiver.sentenceReceived

[lvh]

Hi!

Major update, changed the diff.

Again, any serious review should be against my launchpad branch, not patches. Here's an update anyway.

There's only one or two warts left in nmea.py, I'm not 100% sure how to fix them properly yet, but on the plus side this shouldn't change API when I do figure out how to fix it :-)

[exarkun]

Here is some initial feedback that should keep your todo list from shrinking to nothing for a while. ;) They're based on a pretty superficial look at the code, so none of them are terribly interesting, but they're necessary to address nonetheless:

1. A lot of the code is not covered by the unit tests. You can see this yourself by running trial --coverage twisted.positioning and looking at the files written to _trial_temp/coverage. You should aim for complete line coverage, which means that none of the twisted.positioning.*.cover files in that directory should have any lines beginning with >>>>>> (which indicates the line was not executed during the test suite).
2. twisted/positioning/base.py
   1. The docstrings of BasePositioningReceiver are all pretty short on actual information. The fact that the class is declared to implement IPositioningReceiver tells the reader what all of those methods are implementations of. The class itself could use a docstring explaining that it's a good base for implementations of that interface, and that its methods all are no-ops.
   2. Heading.heading (that's a bit redundant, eh?) and Heading.variation seem to be in degrees, but this isn't documented.
   3. TODO in _clipAngle. Also, it probably doesn't matter, but % is a nice operator, isn't it?
   4. The = used to bind correctHeading needs some whitespace around it.
   5. I don't think @return and @rtype should go into a property doc string. Maybe @type belongs there, or maybe the class should just have @ivar and @type. Mess around with pydoctor a bit to see how different things render.
   6. Take a look at twisted.python.util.FancyEqMixin for defining __eq__
   7. I cast a skeptical eye upon Heading.__imul__
   8. Stray "o" in Speed.__init__.__doc__.
   9. Use symbolic names for latitute and longitude in Coordinate. It improves the docs and reduces the error rate.
   10. What's the range of Coordinate.angle? After the checks in Speed and Heading I expected to see something similar here.
   11. More whitespace in the definition of DOP_EXPRESSIONS
   12. TODO in BeaconInformation
   13. acchieve typo in PositioningBeacon.
   14. TODO in Satellite
3. twisted/positioning/ipositioning.py
   1. climbReceived has a TODO in it.
   2. These receivers might want startup and shutdown methods, analogous to IProtocol.makeConnection and IProtocol.connectionLost.
4. twisted/positioning/nmea.py
   1. from twisted.protocols.basic import LineReceiver would be excellent. ;)
   2. super usage in NMEASentence.__init__ is questionable. What multiple inheritance is this class expected to participate in? And why is it okay to change the signature of __init__ in this super call? Generally that's a big no-no. Also, if this is going to use super, it needs to be documented, since it's part of the public API.
   3. NMEAAdapter is missing some ivar docs.
5. Just in general,
   1. Lots of C{} markup in these docstrings could be L{} markup instead. Use C{} for something you might evaluate (somehow - with Python, in a shell, whatever). Use L{} for the name of a Python object.
   2. No generator expressions allowed. Python 2.3 compatibility is still required for Twisted.
   3. Try to use \ for line continuation less. Parenthesis are your friend here.
   4. __all__ is kinda nice, you probably want to add it to some of these modules
   5. @since 9.0 should go into some module docstrings.

As I said, pretty superficial. Possibly I'll try for a deeper understanding of the code on a future review, although I found things mostly straightforward and well written. So, nice work so far. :)

(I reviewed revision 15356 of  bzr+ssh://bazaar.launchpad.net/~lvh/twisted/positioning/)

[greywulf]

I've tried using the code a bit and I think there's a structural problem in the PositioningReceiver interface.

Currently, splitting the data in several methods
positionReceived...
timeReceived...

loses all association between the data, that is what data belongs to which update.

Use case 1:
Suppose the user wants to gather a track with times included or any other time series. Is timeReceived called before or after the data that it's to be combined with?

Use case 2:
Suppose the user is just interested in positions. What happens if there are multiple NMEA messages with position information? For example, both a RMC and a GGA message (currently positionReceived is called multiple times)

[lvh]

Hi Eero :-)

I've tried using the code a bit and I think there's a structural problem in the PositioningReceiver interface. Currently, splitting the data in several methods positionReceived... timeReceived... loses all association between the data, that is what data belongs to which update.

Yes, these are supposed to return the latest data when they are called -- or to put it in another way, they are supposed to fire whenever . What does update mean in this context? Reporting cycle or sentence?

Use case 1: Suppose the user wants to gather a track with times included or any other time series. Is timeReceived called before or after the data that it's to be combined with?

If I am not mistaken, most GPSes will have reporting cycles that are short enough for this not to be a problem (1Hz or less or something), but perhaps this is not good enough for your particular application :-)

The problem is that a lot of the information that might be stored in time series might be presented in sentences that do not have time information. The most logical thing to do IMHO would be to implement an IPositioningReceiver that remebers the last presented time (eg an instanceattr that gets updated on timeReceived).

It would be possible to run the callbacks per sentence in a particular order, so that *if* the sentence contains a time, it is guaranteed that that is reported first. Again, I'm not 100% sure this will be a problem, but the latter solution will in all cases be at least as good as the data the GPS actually gives you :-)

Use case 2: Suppose the user is just interested in positions. What happens if there are multiple NMEA messages with position information? For example, both a RMC and > a GGA message (currently positionReceived is called multiple times)

Yep, the idea is that NMEAAdapter adapts an NMEAProtocol to an IPositioningReceiver, so it can't ignore things produced by the NMEAProtocol -- it does something with every bit of data (usually this means "fire a callback", sometimes it means "store a bit of stuff in intermediate state", sometimes it means "clear the current state, the gps has no idea where it is anymore"). Ignoring sentences based on some criterion should mostly be the job of the IPositioningReceiver.

If it turns out that people only want positioning, once the receiver is finished, they can always subclass the Adapter so that it doesn't waste time converting a bunch of stuff they don't really need, but that sounds like it should be a last step -- that's purely a performance tweak, not something that should implement new behavior. This is why the Adapter passes all the data from the protocol -- all the behavior is there, you usually want to filter so you get *less* data :-)

Regardless of what we end up doing, this should obviously be documented in the Lore docs (which don't exist yet), since graywulf knows his way around GPS stuff and all of this wasn't obvious from skimming the code (in all fairness, I only grokked it after glyph explained it to me twenty times :-)).

Thanks for your input! Laurens

[greywulf]

Replying to lvh:

Okay, I've reunited with my copy of NMEA 0183.

I've tried using the code a bit and I think there's a structural problem in the PositioningReceiver interface. Currently, splitting the data in several methods positionReceived... timeReceived... loses all association between the data, that is what data belongs to which update.

Yes, these are supposed to return the latest data when they are called -- or to put it in another way, they are supposed to fire whenever . What does update mean in this context? Reporting cycle or sentence?

I was talking about the reporting cycle. You're right, these should be distinguished.

Use case 1: Suppose the user wants to gather a track with times included or any other time series. Is timeReceived called before or after the data that it's to be combined with?

If I am not mistaken, most GPSes will have reporting cycles that are short enough for this not to be a problem (1Hz or less or something), but perhaps this is not good enough for your particular application :-) The problem is that a lot of the information that might be stored in time series might be presented in sentences that do not have time information. The most logical thing to do IMHO would be to implement an IPositioningReceiver that remebers the last presented time (eg an instanceattr that gets updated on timeReceived). It would be possible to run the callbacks per sentence in a particular order, so that *if* the sentence contains a time, it is guaranteed that that is reported first. Again, I'm not 100% sure this will be a problem, but the latter solution will in all cases be at least as good as the data the GPS actually gives you :-)

Timing analysis is generally icky, noisy if you're behind a TCP/IP network connection, and impossible if you are just reading a NMEA logfile.

Actually implementing the current IPositioningReceiver interface would mean maintaining two states:[BR]one for the sentence and one for the report cycle.

Use case 2: Suppose the user is just interested in positions. What happens if there are multiple NMEA messages with position information? For example, both a RMC and > a GGA message (currently positionReceived is called multiple times)

Yep, the idea is that NMEAAdapter adapts an NMEAProtocol to an IPositioningReceiver, so it can't ignore things produced by the NMEAProtocol -- it does something with every bit of data (usually this means "fire a callback", sometimes it means "store a bit of stuff in intermediate state", sometimes it means "clear the current state, the gps has no idea where it is anymore"). Ignoring sentences based on some criterion should mostly be the job of the IPositioningReceiver. If it turns out that people only want positioning, once the receiver is finished, they can always subclass the Adapter so that it doesn't waste time converting a bunch of stuff they don't really need, but that sounds like it should be a last step -- that's purely a performance tweak, not something that should implement new behavior. This is why the Adapter passes all the data from the protocol -- all the behavior is there, you usually want to filter so you get *less* data :-)

As mentioned in here  http://esr.ibiblio.org/?p=801 , the NMEA standard does not specify the order of messages.

I checked the sentences and fortunately sentences with position data always have a UTC timestamp, so you can also always produce a position time series from sentences.

The same is not true for example velocity or satellite data. If you want parse a time series of those from logs, you either get have to know the order of messages, or have to go through the whole packet sniffer business like gpsd, if it's even possible on logs only.

I sketched a bit how I would actually like to use a sentence parser. It's small enough so I'm including it below:

class StatefulParser:
    def __init__(self):
        """The current report cycle is identified by the timestamp"""
        self.currentCycle = NoData
        self.lat = NoData # since null is a valid NMEA value
        self.lon = Nodata

    def process(self, nmealine):
        if not isValid(nmealine):
            return None # skip corrupted lines
        
        sen = Sentence(nmealine)
        t = time(sen)
        clearCycle() if self.currentCycle < t else self.currentCycle = t
        
        if sen.hasPositionData():
            self.lat = DDLat(s)
            self.lon = DDLon(s)

The approach is a bit different. I'd like to just ignore corrupted or unknown types of sentences instead of throwing exceptions. Also DDLat and DDLon have a more 'coercion' flavor, which was supposedly pythonic but apparently isn't in python 3.0

Have to go now, but lemme know what you think. Esr is right though, NMEA truly sucks.

[glyph]

So, it's been 8 months without an update. Is this effort still ongoing? What's the plan?

[lvh]

Hi! See attached patch for review.

Questionable things:

There's a lot of x = property(x) calls that could be replaced by decorators.

A lot of things are defined by a particular implementation in t.p.base because I don't really think they're important enough to warrant an interface. The obvious rebuttal to that is that everything is important enough to warrant an interface. However, this can probably be done later (not in a different ticket, but after review :-)).

Accessing the hemisphere instanceattr on t.p.base.Coordinate instances raises ValueError when accessed on an invalid Coordinate. See t.p.test.test_base.CoordinateTests.badHemisphere. Although that could be classified as a case of GIGO, but maybe it should return None instead.

Support for gpsd was nuked. Can be added later. The old protocol that was partially implemented doesn't work anymore on current releases anyway, so it doesn't really matter.

Support for statistics in positioning was trashed. It's mostly academic anyway. Also, the math involved would be nearly impossible to review. Could be added in a later ticket but I doubt anyone cares.

Coverage is seemingly incomplete, but this is a lie:

Name                                     Stmts   Miss  Cover   Missing
----------------------------------------------------------------------
twisted/positioning/__init__                 0      0   100%   
twisted/positioning/base                   245      0   100%   
twisted/positioning/ipositioning            15      0   100%   
twisted/positioning/nmea                   219      1    99%   621
twisted/positioning/test/__init__            0      0   100%   
twisted/positioning/test/test_base         308      0   100%   
twisted/positioning/test/test_nmea         353      0   100%   
twisted/positioning/test/test_sentence      56      0   100%   
----------------------------------------------------------------------
TOTAL                                     1196      1    99%   

The offending line is the continue in:

if prn is None or snr is None:
    continue

I have asked Ned Batchelder about this. It is the consequence of the peephole optimizer. I have documented it as such in the code, and added a # pragma: no cover to the offending line so that coverage.py will correctly report full coverage.

Finally, _BaseSpeed looks like a last minute tack-on because it is. Unfortunately Climb subclassing Speed was a mistake, because although negative Speeds are nonsensical, negative Climbs just mean you're losing altitude. Oops. I introduced a superclass to have the comparison and unit conversion behavior, which is still identical between the two. __init__ warns about raising ValueError because Liskov.

[lvh]

Oh, yes, and obviously #3953 needs to be merged into this before this can hit trunk...

[lvh]

(In [32412]) Branching to 'positioning-3926'

[lvh]

Buildbot:  http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/positioning-3926

Looks like the failures are unrelated.

[glyph]

At least the documentation builder's failure is relevant:

these 1 objects' docstrings are not proper epytext:
    twisted.positioning.nmea.NMEASentence

[exarkun]

1. Glyph's comment above
2. Some very high-level documentation would be nice. Not necessarily a lot, but something to orient a potential developer. Just something which spells out the roles of the various pieces (PositioningReceivers, PositioningBeacons, Sentences, etc) and how things interact.
3. The AttributeError handling in nmea.NMEAProtocol.lineReceived is wrong. It will catch exceptions from the callable. Just wrap the getattr, or use the three argument form of getattr.
4. Documentation for PositionError.ALLOWABLE_TRESHOLD is missing.
5. Some lines in the code are a little long, 81 columns. Should be 80 columns max.
6. PositioningBeacon.__init__ and Satellite.__init__ are not compatible enough for super to be safe to use with them. A straight upcall by base class name in Satellite.__init__ does the job just as well.
7. Heading.variation isn't documented well enough for me (someone mostly ignorant of GPS) to know for sure what it represents. I'm sure it's well defined, so perhaps a reference to an external resource defining it would help? This sort of comment could probably apply to many of the things in this branch.
8. A couple lines in nmea.py are missing test coverage (legitimately, I think, as compared to many lines bogusly reported as uncovered there and elsewhere :/)
9. in NMEAAdapter._fixTimestamp, maybe string formatting isn't necessary? What's wrong with integer addition?
10. I guess we should use twisted.python.compat.reduce instead of the builtin, to avoid some py3k warning I think.
11. An update to the gps example in doc/core/examples/ to use these new APIs instead of the old ones would be pretty nice.

Overall, great job on test coverage, great docstrings, great job keeping the math easy to follow. I think I might not have gone the same way with the inMetersPerSecond/inDegreesMinutesSeconds/etc sort of properties, but there's no standard units library so I guess it's hard to do better than that.

[exarkun]

Oh, I guess I should point out that while the code looks good, I didn't actually test it against any real GPS devices. :)

[glyph]

Replying to exarkun:

Oh, I guess I should point out that while the code looks good, I didn't actually test it against any real GPS devices. :)

This begs the question - where might one get the raw data stream from such a thing? Is there any cost-effective means to do a little manual testing with this? (In the future, perhaps we could strap a Buildbot to the roof of a car...)

[lvh]

(In [37602]) Branching to 'positioning-3926-2'

[lvh]

The AttributeError handling in nmea.NMEAProtocol.lineReceived is wrong. It will catch exceptions from the callable. Just wrap the getattr, or use the three argument form of getattr.

Addressed in  https://twistedmatrix.com/trac/changeset/37609

[lvh]

Documentation for PositionError.ALLOWABLE_TRESHOLD is missing.

Addressed in  https://twistedmatrix.com/trac/changeset/37613

[lvh]

PositioningBeacon.init and Satellite.init are not compatible enough for super to be safe to use with them. A straight upcall by base class name in Satellite.init does the job just as well.

Addressed in  https://twistedmatrix.com/trac/changeset/37614

[lvh]

in NMEAAdapter._fixTimestamp, maybe string formatting isn't necessary? What's wrong with integer addition?

I can't figure out what exarkun meant by this at the time, and showing it to him now didn't illuminate either of us, so I'm going to go with "it's fine the way it is"

[lvh]

I guess we should use twisted.python.compat.reduce instead of the builtin, to avoid some py3k warning I think.

Addressed in  https://twistedmatrix.com/trac/changeset/37620

[lvh]

The new _ALLOWABLE_THRESHOLD docstring didn't correctly indent lines. I also cleaned it up a little:  https://twistedmatrix.com/trac/changeset/37624

[lvh]

twisted-checker complains about this:

W9202: 37,4:BasePositioningReceiver.timeReceived: Missing epytext markup @param for argument "time" W9202: 43,4:BasePositioningReceiver.headingReceived: Missing epytext markup @param for argument "heading" W9202: 49,4:BasePositioningReceiver.speedReceived: Missing epytext markup @param for argument "speed" W9202: 55,4:BasePositioningReceiver.climbReceived: Missing epytext markup @param for argument "climb" W9202: 61,4:BasePositioningReceiver.positionReceived: Missing epytext markup @param for argument "latitude" W9202: 61,4:BasePositioningReceiver.positionReceived: Missing epytext markup @param for argument "longitude" W9202: 67,4:BasePositioningReceiver.positionErrorReceived: Missing epytext markup @param for argument "positionError" W9202: 73,4:BasePositioningReceiver.altitudeReceived: Missing epytext markup @param for argument "altitude" W9202: 79,4:BasePositioningReceiver.beaconInformationReceived: Missing epytext markup @param for argument "beaconInformation"

That's technically true, but the are documented on the interface. Should I copy all of the ones from the interface? That seems duplicitous.

[lvh]

I think that I addressed the "variation is underdocumented" issue by simply making the docstring say "magnetic variation" -- in that case, at least you have something to Google for :)

I also think that addresses everything minus the docs is OK, so perhaps now would be a good time to put "review" on it? :)

[therve]

• @since are either referring to 13.0 or 11.1. Sorry you missed the window.

• There should be a __all__ attribute in the python modules. Maybe make anything which isn't in there private.

• You're using both @property and property(), you should settle on one.

• @raise ValueErorr: typo in setSign

•           try:
                sign = int(self.inDecimalDegrees < 0)
                return self.HEMISPHERES_BY_TYPE_AND_SIGN[self.angleType][sign]
            except KeyError:
  

  Maybe put the int cast outside of the try/except.

• I feel I should complain about the __float__ methods, maybe? It seems they are already replaced by explicit methods anyway.

• Climb.__init__ says that it raises ValueError, but it doesn't.

• @return: C{None} if the invariant was not satisifed or not tested. Typo

• used in acquiring a postioning fix. Typo

• beacons of which it is unknown if they Extra space before unknown

• self.beacons = set(beacons or []) Not super fan of the or call here, but whatever

•       def _getNumberOfBeaconsUsed(self):
            """
            Returns the number of beacons that can be seen.
    
            @return: The number of beacons that can be seen, or C{None} if the number
                is unknown. This happens as soon as one of the beacons has an unknown
                (C{None}) C{isUsed} attribute.
  

The method says used, but the docstring says seen?

• @ivar identifier: The unqiue identifier typo

• It feels that the FIXERS could be defined a bit more simply, without lambdas. What about a tuple with method and the arguments?

I'm hungry! Hope it helped.

[lvh]

@since are either referring to 13.0 or 11.1. Sorry you missed the window.

Fixed in  https://twistedmatrix.com/trac/changeset/37743

@return: C{None} if the invariant was not satisifed or not tested. Typo

Fixed in  https://twistedmatrix.com/trac/changeset/37744

used in acquiring a postioning fix. Typo

Fixed in  https://twistedmatrix.com/trac/changeset/37745

beacons of which it is unknown if they Extra space before unknown

Fixed in  https://twistedmatrix.com/trac/changeset/37747

@ivar identifier: The unqiue identifier typo

Fixed in  https://twistedmatrix.com/trac/changeset/37748

self.beacons = set(beacons or []) Not super fan of the or call here, but whatever

Fixed in  https://twistedmatrix.com/trac/changeset/37749

You're using both @property and property(), you should settle on one.

Ah, yeah, sorry about that; back when I started writing this I couldn't use decorators yet in Twisted, so I started fixing it wherever I could ;) There are a few like these left:

positioning ❯ grin "= property"
./base.py:
  843 :     pdop = property(fget=lambda self: self._getDOP('pdop'),
  847 :     hdop = property(fget=lambda self: self._getDOP('hdop'),
  851 :     vdop = property(fget=lambda self: self._getDOP('vdop'),

... but those appear to make sense. Addressed in  https://twistedmatrix.com/trac/changeset/37753

The method says used, but the docstring says seen?

Oops! Fixed in  https://twistedmatrix.com/trac/changeset/37754

@raise ValueErorr: typo in setSign

Addressed in  https://twistedmatrix.com/trac/changeset/37755

This leaves the following review points:

* It feels that the FIXERS could be defined a bit more simply, without lambdas. What about a tuple with method and the arguments? * There should be a all attribute in the python modules. Maybe make anything which isn't in there private. * Climb.init says that it raises ValueError, but it doesn't. * I feel I should complain about the float methods, maybe? It seems they are already replaced by explicit methods anyway.

... and, of course, the missing high level documentation :)

I noticed some bad API myself... For example I don't know why Heading.setSign just defers to self.variation.setSign... I think this is a consequence of a some bad inheritance hierarchies. I'll have to ponder that.

[lvh]

I've also moved the sign computation out of the try/except block:  https://twistedmatrix.com/trac/changeset/37758

[lvh]

Climb.init says that it raises ValueError, but it doesn't.

Pondered it, that's a bug; removed the reference, negative climbs are totally okay;  https://twistedmatrix.com/trac/changeset/37759

[therve]

Yay progress. Some comments on the tests:

• We settled on assertEqual, instead of assertEquals.
• The test docstrings are not great. I suppose now it'd be a pain to change them all, but try to avoid "Tests that..." and "Check that...".
• There are a bunch of tests with asserts, it'd be nice to have something for them.
• There are a bunch of tests which do assert in a for loop. Those are pretty painful to debug later on, I wonder if some could be split.

Thanks!

[lvh]

Thanks for the comments! Can you elaborate on (3)? Aren't tests supposed to have asserts?

[therve]

Duh. There are a bunch of tests without asserts.

[lvh]

I think I've fixed a bunch of these, at least the ones were the improvement was obvious. Are there any remaining ones you don't like?

(I'd re-submit this for review but it seems the review keyword was never removed)

[therve]

1.

DATA_ACTIVE, DATA_VOID = "A", "V"

# Selection modes (used in a variety of sentences):
MODE_AUTO, MODE_MANUAL = 'A', 'M'

# GPGSA fix types:
GSA_NO_FIX, GSA_2D_FIX, GSA_3D_FIX = '1', '2', '3'

NMEA_NORTH, NMEA_EAST, NMEA_SOUTH, NMEA_WEST = "N", "E", "S", "W"

Please swith to single quotes consistently.

2. Yay documentation. I don't think you should create a top directory though, but instead a file in doc/core/howto/, linked in the index. Also moving the example in the listings directory.

3. In the documentation, I don't think referring to the code as "Twisted Positioning" is particularly good. Maybe just a <code> is enough.

4. There are several pydoctor errors, with a blocking one in NMEASentence.

5. I don't really see the point of _IPositioningSentenceProducer. In particular nothing fails if NMEAProtocol doesn't provide it anymore. Same for INMEAReceiver.

6. There are a bunch of TODOs in test_base.

Almost there!

[lvh]

1. Does that mean "use single quotes" or "pick either single or double quotes, but pick one and use it consistently"?

[lvh]

(6) was addressed in r37996, (1) was addressed in r37997.

(5) _IPositioningSentenceProducer is definitely vestigial from when I tried to support the classic gpsd protocol, which is basically a sanitized NMEA. However, that protocol is deprecated these days (in gpsd), so I ripped it out. It *might* later on be useful for Zodiac, but my Zodiac is sufficiently rusty that I'm not sure. Supporting the new gpsd protocol is definitely a priority over supporting Zodiac natively.

INMEAReceiver is there so that people who have to deal with truly crazy NMEA chips that basically spout complete nonsense and that could not be supported without breaking other, standards-compliant chips.

[therve]

Should it be back up for review?

[lvh]

I don't think so, I haven't done the two documentation points and _IPositioningSentenceProducer is still there. (I've been on holiday this week, returning tomorrow.)

[lvh]

I axed the interface mentioned in point (5) in r38385. The example was moved to the listings directory in r38386, the documentation as a whole was moved under core/ in r38387, covering (2). (3) was fixed in r38388.

Resubmitting :)