Opened 5 years ago

Closed 8 months ago

#3926 enhancement closed fixed (fixed)

twisted.positioning -- a better positioning framework

Reported by: lvh Owned by: lvh
Priority: normal Milestone:
Component: core Keywords: protocols gps positioning
Cc: ashfall@… Branch: branches/positioning-3926-3
(diff, github, buildbot, log)
Author: hawkowl, lvh Launchpad Bug:

Description (last modified by 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 (#6810)
  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.

Attachments (2)

positioning.diff (75.2 KB) - added by lvh 5 years ago.
positioning-3926.patch (135.1 KB) - added by lvh 3 years ago.

Download all attachments as: .zip

Change History (85)

comment:1 Changed 5 years ago by lvh

  • Summary changed from twisted.protocols.positioning -- a better positioning frameworkjk to twisted.protocols.positioning -- a better positioning framework

comment:2 Changed 5 years ago by 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.)

comment:3 Changed 5 years ago by 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.

comment:4 Changed 5 years ago by 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?

comment:5 Changed 5 years ago by 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.

comment:6 Changed 5 years ago by lvh

  • Keywords review removed
  • Owner changed from glyph to lvh

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

comment:7 Changed 5 years ago by lvh

  • Keywords review added
  • Owner lvh deleted

Hi!

Patch up for review, enjoy.

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

comment:8 Changed 5 years ago by 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


comment:9 Changed 5 years ago by 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 :-)

Changed 5 years ago by lvh

comment:10 Changed 5 years ago by exarkun

  • Keywords review removed
  • Owner set to lvh

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/)

comment:11 Changed 5 years ago by 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)

comment:12 follow-up: Changed 5 years ago by 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

comment:13 in reply to: ↑ 12 Changed 5 years ago by 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.

comment:14 Changed 5 years ago by glyph

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

Changed 3 years ago by lvh

comment:15 Changed 3 years ago by lvh

  • Keywords review added

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.

comment:16 Changed 3 years ago by lvh

  • Owner lvh deleted

comment:17 Changed 3 years ago by lvh

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

comment:18 Changed 3 years ago by lvh

  • Description modified (diff)
  • Summary changed from twisted.protocols.positioning -- a better positioning framework to twisted.positioning -- a better positioning framework

comment:19 Changed 3 years ago by lvh

  • Branch set to branches/positioning-3926

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

comment:21 Changed 3 years ago by glyph

At least the documentation builder's failure is relevant:

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

comment:22 Changed 3 years ago by exarkun

  • Keywords review removed
  • Owner set to lvh
  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.

comment:23 follow-up: Changed 3 years ago by 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. :)

comment:24 in reply to: ↑ 23 Changed 3 years ago by 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...)

comment:25 Changed 21 months ago by lvh

  • Branch changed from branches/positioning-3926 to branches/positioning-3926-2

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

comment:26 Changed 21 months ago by 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

comment:27 Changed 21 months ago by lvh

Documentation for PositionError.ALLOWABLE_TRESHOLD is missing.

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

comment:28 Changed 21 months ago by 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

comment:29 Changed 21 months ago by 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"

comment:30 Changed 21 months ago by 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

comment:31 Changed 21 months ago by lvh

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

comment:32 Changed 21 months ago by 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.

comment:33 Changed 21 months ago by lvh

  • Keywords review added
  • Owner lvh deleted

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? :)

comment:34 Changed 21 months ago by 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.

comment:35 Changed 21 months ago by 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.

comment:36 Changed 21 months ago by lvh

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

comment:37 Changed 21 months ago by 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

comment:38 Changed 20 months ago by 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!

comment:39 Changed 20 months ago by lvh

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

comment:40 Changed 20 months ago by therve

Duh. There are a bunch of tests without asserts.

comment:41 Changed 20 months ago by 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)

comment:42 Changed 20 months ago by therve

  • Owner set to therve

comment:43 Changed 20 months ago by therve

  • Keywords review removed
  • Owner changed from therve to lvh

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.

  1. 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.
  1. In the documentation, I don't think referring to the code as "Twisted Positioning" is particularly good. Maybe just a <code> is enough.
  1. There are several pydoctor errors, with a blocking one in NMEASentence.
  1. I don't really see the point of _IPositioningSentenceProducer. In particular nothing fails if NMEAProtocol doesn't provide it anymore. Same for INMEAReceiver.
  1. There are a bunch of TODOs in test_base.

Almost there!

comment:44 Changed 20 months ago by lvh

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

comment:45 Changed 20 months ago by 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.

comment:46 Changed 20 months ago by therve

Should it be back up for review?

comment:47 Changed 20 months ago by 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.)

comment:48 Changed 19 months ago by lvh

  • Keywords review added

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

comment:49 Changed 19 months ago by lvh

  • Owner lvh deleted

comment:50 Changed 18 months ago by tom.prince

  • Keywords review removed
  • Owner set to lvh
  1. The interfaces here make the assumption that geocentric positioning is required. We will need at least helio-centric positioning to resolve #5000. Should the interface be changed or renamed to take this into account. ;)
  2. There appears to be 5 failing tests.
  3. Is the sentence handling code generic enough to be used by multiple protocols, or is it a detail of the NMEA implementation? In the former, it perhaps wants to be in its own module (_sentence maybe?) and the later, in NMEA.
    • If we anticipate people writing their own protocols outside twisted, we'll want to make this public. But I think it is reasonable to wait until there are at least a couple of users, before exposing them to compatibility guarantee.
    • Also, is sentence standard terminology for this?
    • I notice that the tests are already somewhat split out.
  4. I wonder if the different angleTypes should be different classes, with a common, (private?) base. Also, should there ever be an Angle of type latitude or longitude that isn't a Coordinate?
  5. It isn't clear to me why changing the sign is a valid thing to do. It looks like this is used by the protocol implementations, but should the end-user ever be doing it?
  6. _PositioningSentenceProducerMixing should explain *how* it helps provide IPositioningSentenceProducingProtocol.
  7. Does BeaconInformation.seen and BeaconInformation.used provide value over just doing len() to appropriate thing?
  8. The BeaconInformation.beacons and BeaconInformation.usedBeacons return different types. Is there a reason for this? Also, __iter__ could probably just be return iter(self.beacons).
    • Also, what if beacons don't have unique identifiers?
  9. PositioningBeacon doesn't implement IPositioningBeacon. (There should be a test for this.) On further inspection, that interface doesn't exist but is referred to.
  10. I wonder if isUsed should be an attribute of PositioningBeacon or part of BeaconInformation. I would be inclined to suspect that PositioningBeacons are stable over time.
  11. The docstring for nmea should provide some references.
  12. @param rawSentence: The MMEA positioning sentence. typo?
  13. Should something validate that sentences start with $?
  14. NMEAProtocol.lineReceived calls nmea_<sentenceType> but doesn't provide any of those, and provides a more sensible (i.e. composition instead of inheritance) way to interact with it. I think the support for nmea_ prefixed methods should be removed.
  15. # The next parts are DGPS information. seems to be missing the next parts after it.
  16. DATESTAMP_HANDLING as currently implemented should use twisted.python.constants. But the current code will break when dates start being in 2200.
    1. In any case, this should be (and the code treats it as) a instance, not a class variable. Somebody could connect to two different devices, that need different handling in a single program.
    2. Another option, would be to just provide a year, that represents the earlies possible date. So the current default behaviour would be DATE_THRESHOLD = 1980', DATESTAMPS_FROM_20XX would be DATE_THRESHOLD = 2000 and DATESTAMPS_FROM_19xx would be DATE_THRESHOLD = 1900. (I'm not suggesting that DATE_THRESHOLD` is an appropriate name, though).
  17. NMEAAdapter is missing docstrings for a bunch of class and instance attributes.
  18. Is there a reason that miles are the preferred units?
  19. .strip('Units') probably doesn't do what you want it to do (although it will work sometimes).
  20. NMEAAdapter._updateSentence is confusingly named, as it doesn't affect the sentence.
  21. I haven't followed all the logic closely, but a bunch of the logic in NMEAAdapter seems like it could be made stateless. But perhaps this would make the code harder to follow.
  22. Why is NMEAAdapter.REQUIRED_CALLBACK_FIELDS set outside the class?
  23. Is there a reason for the constants in nmea and class attributes of NMEAAdapter and NMEAProtocol to to be public? Many of them seem to be implementation details.
    • And a number of them seem to be candidates for twisted.python.constants.Values.
  24. MockNMEAReceiver isn't actually NMEA specific.
  25. FixerTestMixin._fixerTest doesn't seem private. Maybe it could have a more descriptive name (or be split into two functions with descriptive names) too.
    • Same for ParsingTests._parserTest.
  26. I'd be more confortable if CoordinateFixerTests used explicit data, rather than calculated values. As it is, you are testing that the fixers do the opposite of the conversion present in the test module. It would be better to give explicit values for before and after.

comment:51 Changed 18 months ago by tom.prince

Oh, and the ibiblio link should likely be somewhere in the docs.

comment:52 Changed 18 months ago by lvh

  • Keywords review added
  • Owner lvh deleted

Replying to tom.prince:

  1. The interfaces here make the assumption that geocentric positioning is required. We will need at least helio-centric positioning to resolve #5000. Should the interface be changed or renamed to take this into account. ;)

Heh. Suggestions welcome :-)

  1. There appears to be 5 failing tests.

They appeared to be failing due to implicitly numbered format strings not working on 2.6. Fixed in r38467.

  1. Is the sentence handling code generic enough to be used by multiple protocols, or is it a detail of the NMEA implementation? In the former, it perhaps wants to be in its own module (_sentence maybe?) and the later, in NMEA.

Maybe. It used to be there for the old ("classic", now deprecated) GPSD protocol. It certainly was able to do that. Support for that protocol was killed because upstream deprecated it. I figured leaving it in as an implementation detail is a good idea, since it might help later. If it doesn't, we can just kill it.

  • If we anticipate people writing their own protocols outside twisted, we'll want to make this public. But I think it is reasonable to wait until there are at least a couple of users, before exposing them to compatibility guarantee.

Right, see above.

  • Also, is sentence standard terminology for this?

Yes. NMEA uses it, gpsd uses it. The big binary protocols, SiRF and Zodiac (actually a brand name for a particular Rockwell chip, not so much a protocol, it's just the most popular thing that uses that protocol -- it's also called the Conexant and NAVMAN protocol for similar reasons) seem to use "binary message". "Sentence" is used to mean generic things (including when they could potentially be messages from SiRF and Zodiac); OTOH, I don't think "message" is never used to refer to NMEA sentences.

  • I notice that the tests are already somewhat split out.

Ehh, yes, I suppose? :)

  1. I wonder if the different angleTypes should be different classes, with a common, (private?) base. Also, should there ever be an Angle of type latitude or longitude that isn't a Coordinate?

No, there shouldn't ever be such a thing. ISTR I tried to fix this, and it pretty much blew up in my face. I guess I should've done a better job writing down why. At the very least, it was a ton of work before I gave up :)

  1. It isn't clear to me why changing the sign is a valid thing to do. It looks like this is used by the protocol implementations, but should the end-user ever be doing it?

Probably not -- if they do, multiplying by -1 should be good enough. Do you want me to remove it?

  1. _PositioningSentenceProducerMixing should explain *how* it helps provide IPositioningSentenceProducingProtocol.

Argh. I wanted this to be more internal and implementation specific.

  1. Does BeaconInformation.seen and BeaconInformation.used provide value over just doing len() to appropriate thing?

No, not really. Do you want me to remove it?

  1. The BeaconInformation.beacons and BeaconInformation.usedBeacons return different types. Is there a reason for this? Also, __iter__ could probably just be return iter(self.beacons).

Not that I can think of -- I don't think there's a reason usedBeacons couldn't return a set (and, in fact, keep its signature). Do you want me to make both return sets?

I'm inclined towards making BeaconInformation not iterable at all, if the API does change; if it's not obvious if it should be seen or used beacons it should probably be neither.

  • Also, what if beacons don't have unique identifiers?

I don't know of any systems that satisfy this. The best I can think of are RFID/NFC things that work by walking near them, but in that case their location could serve as a unique identifier.

If they don't have useful unique identifiers, then, depending on the situation, I guess you could pretend that all of them map to the same identifier and just pick one, or you make a random one for each.

  1. PositioningBeacon doesn't implement IPositioningBeacon. (There should be a test for this.) On further inspection, that interface doesn't exist but is referred to.

Fixed in r38472.

  1. I wonder if isUsed should be an attribute of PositioningBeacon or part of BeaconInformation. I would be inclined to suspect that PositioningBeacons are stable over time.

I feel pretty strongly that the information on whether or not it is used is part of the beacon information and not of the beacon itself. That information only exists when combining the data into something (i.e., the beacon information), not for indivudual beacons. So, isUsed is an implementation detail.

  1. The docstring for nmea should provide some references.

Unfortunately, NMEA 0183 is not a publicly available standard. I've documented this, together with the ESR blog post, in the docstring. Fixed in r38468.

  1. @param rawSentence: The MMEA positioning sentence. typo?

Fixed in r38463.

  1. Should something validate that sentences start with $?

I'm not sure what appropriate behavior would be to deal with that. I think some (mostly fake) GPS devices produce buggy sentences like that, but mostly only for fuzzing parsers.

  1. NMEAProtocol.lineReceived calls nmea_<sentenceType> but doesn't provide any of those, and provides a more sensible (i.e. composition instead of inheritance) way to interact with it. I think the support for nmea_ prefixed methods should be removed.
  2. # The next parts are DGPS information. seems to be missing the next parts after it.

Right. DGPS information isn't used, so the parts are unspecified. That means they just get ignored. Alternatively, I could put a bunch of Nones there, which I think is the way it used to be.

  1. DATESTAMP_HANDLING as currently implemented should use twisted.python.constants. But the current code will break when dates start being in 2200.

Wouldn't the code start doing wrong things in the future anyway, since the year is only specified as 2 bytes?

  1. In any case, this should be (and the code treats it as) a instance, not a class variable. Somebody could connect to two different devices, that need different handling in a single program.

Okay. Since the code already treats it that way, I'm guessing the only required change is one of documentation?

  1. Another option, would be to just provide a year, that represents the earlies possible date. So the current default behaviour would be DATE_THRESHOLD = 1980', DATESTAMPS_FROM_20XX would be DATE_THRESHOLD = 2000 and DATESTAMPS_FROM_19xx would be DATE_THRESHOLD = 1900. (I'm not suggesting that DATE_THRESHOLD` is an appropriate name, though).

Okay, that sounds reasonable; if the above thing is clarified I'm amenable to implementing it.

  1. NMEAAdapter is missing docstrings for a bunch of class and instance attributes.

Fixed in r38469.

  1. Is there a reason that miles are the preferred units?

They shouldn't be. What are you referring to? If it's "M", I believe that is NMEA-speak for meters per second, not miles.

  1. .strip('Units') probably doesn't do what you want it to do (although it will work sometimes).

Oh, wow. That one's just... Wow. Yeah. Oops? :-) Fixed in r38465.

  1. NMEAAdapter._updateSentence is confusingly named, as it doesn't affect the sentence.

Fixed in r38466.

  1. I haven't followed all the logic closely, but a bunch of the logic in NMEAAdapter seems like it could be made stateless. But perhaps this would make the code harder to follow.

Possibly! Do you have any particular one in mind? I'm reasonably sure that there's no sensible way to make it entirely stateless because of GPGSV, so it's possible that I just gave up and used state as a crutch in places where it isn't strictly necessary since I knew I'd had to have it anyway.

  1. Why is NMEAAdapter.REQUIRED_CALLBACK_FIELDS set outside the class?

I don't remember. It doesn't appear to break if I move it, so fixed in r38470.

  1. Is there a reason for the constants in nmea and class attributes of NMEAAdapter and NMEAProtocol to to be public? Many of them seem to be implementation details.

Well, they're part of the NMEA specification, not really implementations details of the module -- but sure, they could be made private.

  • And a number of them seem to be candidates for twisted.python.constants.Values.

This is another one where I tried and gave up because it was a bunch of work for no obvious benefit.

  1. MockNMEAReceiver isn't actually NMEA specific.

Good catch. Renamed and moved to its own module in r38471.

  1. FixerTestMixin._fixerTest doesn't seem private. Maybe it could have a more descriptive name (or be split into two functions with descriptive names) too.
    • Same for ParsingTests._parserTest.

Uh, sure. Does that have to be addressed in this ticket?

  1. I'd be more confortable if CoordinateFixerTests used explicit data, rather than calculated values. As it is, you are testing that the fixers do the opposite of the conversion present in the test module. It would be better to give explicit values for before and after.

Okay, fixed in r38478.

... and finally:

Replying to tom.prince:

Oh, and the ibiblio link should likely be somewhere in the docs.

Fixed in r38468. That is: that comment adds it to the docstring. Do you think it should be in the prose docs as well? (I don't really see the utility.)

comment:53 Changed 18 months ago by tom.prince

  1. The docstring for nmea should provide some references.

Unfortunately, NMEA 0183 is not a publicly available standard. I've documented this, together with the ESR blog post, in the docstring. Fixed in r38468.

Even if they are only available at cost, we shold provide a reference: the name of the specification (NMEA 0183?) and a link to either an official source or wikipedia.

  1. NMEAProtocol.lineReceived calls nmea_<sentenceType> but doesn't provide any of those, and provides a more sensible (i.e. composition instead of inheritance) way to interact with it. I think the support for nmea_ prefixed methods should be removed.

This wasn't addressed

  1. # The next parts are DGPS information. seems to be missing the next parts after it.

Right. DGPS information isn't used, so the parts are unspecified. That means they just get ignored. Alternatively, I could put a bunch of Nones there, which I think is the way it used to be.

  1. DATESTAMP_HANDLING as currently implemented should use twisted.python.constants. But the current code will break when dates start being in 2200.

Wouldn't the code start doing wrong things in the future anyway, since the year is only specified as 2 bytes?

Adding a DATESTAMPS_FROM_21XX or INTELGIENT_DATESTAMS_AROUND_2100 would allow handling of dates in the future.

15.1. In any case, this should be (and the code treats it as) a instance, not a class variable. Somebody could connect to two different devices, that need different handling in a single program.

Okay. Since the code already treats it that way, I'm guessing the only required change is one of documentation?

Yes.

15.2. Another option, would be to just provide a year, that represents the earlies possible date. So the current default behaviour would be DATE_THRESHOLD = 1980', DATESTAMPS_FROM_20XX would be DATE_THRESHOLD = 2000 and DATESTAMPS_FROM_19xx would be DATE_THRESHOLD = 1900. (I'm not suggesting that DATE_THRESHOLD` is an appropriate name, though).

Okay, that sounds reasonable; if the above thing is clarified I'm amenable to implementing it.

I'm not sure what you want clarified?

  1. _PositioningSentenceProducerMixing should explain *how* it helps provide IPositioningSentenceProducingProtocol.

Argh. I wanted this to be more internal and implementation specific.

Even if it is internal, there should be documentation on how it is used.

  1. Does BeaconInformation.seen and BeaconInformation.used provide value over just doing len() to appropriate thing?

No, not really. Do you want me to remove it?

Sure. They can always be added later, but removing them is more difficult.

  1. The BeaconInformation.beacons and BeaconInformation.usedBeacons return different types. Is there a reason for this? Also, __iter__ could probably just be return iter(self.beacons).

Not that I can think of -- I don't think there's a reason usedBeacons couldn't return a set (and, in fact, keep its signature). Do you want me to make both return sets?

I don't have a strong opinion on what the return type is, as long as it is consistent.

I'm inclined towards making BeaconInformation not iterable at all, if the API does change; if it's not obvious if it should be seen or used beacons it should probably be neither.

That makes sense.

  1. Is the sentence handling code generic enough to be used by multiple protocols, or is it a detail of the NMEA implementation? In the former, it perhaps wants to be in its own module (_sentence maybe?) and the later, in NMEA.

Maybe. It used to be there for the old ("classic", now deprecated) GPSD protocol. It certainly was able to do that. Support for that protocol was killed because upstream deprecated it. I figured leaving it in as an implementation detail is a good idea, since it might help later. If it doesn't, we can just

kill it.
That sounds resonable. Lets move it into _sentence then?

  1. It isn't clear to me why changing the sign is a valid thing to do. It looks like this is used by the protocol implementations, but should the end-user ever be doing it?

Probably not -- if they do, multiplying by -1 should be good enough. Do you want me to remove it?

Let's keep the interface small.

  1. I wonder if isUsed should be an attribute of PositioningBeacon or part of BeaconInformation. I would be inclined to suspect that PositioningBeacons are stable over time.

I feel pretty strongly that the information on whether or not it is used is part of the beacon information and not of the beacon itself. That information only exists when combining the data into something (i.e., the beacon information), not for indivudual beacons. So, isUsed is an implementation detail.

If isUsed is an implementation detail, then it shold be private. Or, what I was implicitly suggesting, is that BeaconInformation should store this information, rather than storing it on PositioningBeacon.

  1. I wonder if the different angleTypes should be different classes, with a common, (private?) base. Also, should there ever be an Angle of type latitude or longitude that isn't a Coordinate?

No, there shouldn't ever be such a thing. ISTR I tried to fix this, and it pretty much blew up in my face. I guess I should've done a better job writing down why. At the very least, it was a ton of work before I gave up :)

class Lattitude(_Coordinate):
   def __init__(self, angle):
	_Coordinate.__init__(self, angle, Angles.LATITUDE)

?

  1. Is there a reason for the constants in nmea and class attributes of NMEAAdapter and NMEAProtocol to to be public? Many of them seem to be implementation details.

Well, they're part of the NMEA specification, not really implementations details of the module -- but sure, they could be made private.

The following all seem to be implementation details (and quite a few of them aren't documented either)

  • RANGE_EXPRESSIONS
  • ANGLE_TYPE_NAMES ?
  • HEMISPHERS_BY_TYPE_AND_SIGN
  • DOP_EXPRESSIONS
  • METHOD_PREFIX
  • SENTENCE_CONTENTS ?
  • ALLOWED_ATTRIBUTES ?

The following

  • REQUIRED_CALLBACK_FIELDS
  • FIXERS
  • COORDINATE_SIGNS
  • STATEFUL_UPDATE
  • ACCEPTABLE_UNITS
  • UNIT_CONVERTERS
  • GSV_KEYS
  • SPECFIC_SENTENCE_FIXES
  • And a number of them seem to be candidates for twisted.python.constants.Values.

This is another one where I tried and gave up because it was a bunch of work for no obvious benefit.

class GPGSAFixTypes(Values):
    GSA_NO_FIX = ValuesConstant("1")
    GSA_2D_FIX = ValuesConstant("2")
    GSA_3D_FIX = ValuesConstant("3")

FIXERS = {
  ...
  'fixType' = lambda self: self._convert('fixType', GSPAFixType.lookupByValue),
}

?

comment:54 Changed 18 months ago by tom.prince

  1. _fixUnits:
    1. It doesn't appear that the direct tests of _fixUnits cover it completely.
    2. I seems like the code doesn't handle the case where unit in self.ACCEPTABLE_UNITS
  2. NMEAAdapter
    1. REQUIRED_CALLBACK_FIELDS requires a @cvar
    2. (optional) _fireSentenceCallbacks will fail trying to call None, if a required callback isn't found.
      • It could just silently drop the message. (This would be the same as inheriting from BasePositioningReceiver, without requiring inheritience)
      • It could raise a meaningful error, rather than an implementation detail.
      • NMEAAdapter could check that the required callback are present to begin with (using verifyObject?). (I think this may be the worst of the suggestions.
  3. PositioningBeaconTests.test_interface should use zope.interface.verify.verifyObject (in addition?). (#6555)
  4. _coordinateFixerTest is now unused.


comment:55 Changed 18 months ago by tom.prince

  • Keywords review removed
  • Owner set to lvh

That isn't a complete review, but I need to take a break ...

comment:56 Changed 18 months ago by lvh

Thanks for your continued review efforts. Unfortunately I will not have time to act on them immediately due to a death in the family. I will get back to this as soon as possible.

comment:57 Changed 18 months ago by ashfall

  • Cc ashfall@… added

comment:58 Changed 18 months ago by lvh

Changes

  1. Added the requested references in the t.p.nmea docstring. r38751
  2. (Re-?)added the Nones to the GPGGA sentence definition, with comments as to what they mean. r38752
  3. Moved a bunch of constants to to t.python.constants.Values. GPGSA fix types: r38753, GPGGA fix types: r38754, GPGLL/GPRMC fix qualities: r38757
  4. Removed _coordinateFixerTest. r38759

Notes/questions

  1. In your example using Values, you mention a fixType fixer -- I'm assuming that's just an example, because fixTypes aren't supposed to have fixers. They're only used to throw away bogus data, at that point there's nothing to fix :)

Todo list:

  1. Customization of NMEAProtocol through composition rather than inheritance.
  2. Datestamp handling
  3. Documenting all of those variables in the implementation detail list
  4. Documenting _PositioningSentenceProducerMixin
  5. Documenting on NMEAAdapter: REQUIRED_CALLBACK_FIELDS
  6. Changing the way NMEAAdapter calls methods; I think that's a great idea, I'm not sure if it should be made part of the spec or if it could be (after all, you *are* providing an object that doesn't provide the full interface I claim to need...)
  7. Remove setSign
  8. PositioningBeaconTests.test_interface should use zope.interface.verify.verifyObject (in addition?). (#6555)
  9. Adding Angle types
  10. All of the mentioned changes to PositioningBeacon and BeaconInformation. I'm going to make all return types frozensets. Since len is going away, len(bi.used)/len(bi.seen) ought to be a thing -- obviously that won't work on arbitrary iterators :)
  11. Move generic sentence handling code to _sentence module
  12. _fixUnits

... and I think that's all of them. Whew, that's a long list :)

comment:59 Changed 18 months ago by tom.prince

In your example using Values, you mention a fixType fixer -- I'm assuming that's just an example, because fixTypes aren't supposed to have fixers. They're only used to throw away bogus data, at that point there's nothing to fix :)

Yes. I didn't think about that example code too hard. It was just a sketch. :)

comment:60 follow-up: Changed 18 months ago by lvh

Fixed datestamp handling in r38773.

I figured out why setSign exists: _angle, the value you'd have to change, is private. This is because the entire object is intended to be immutable. I presume that making it mutable is less ugly than the two alternatives, which are:

  1. Use the _angle implementation detail in the NMEA implementation
  2. Keep setSign.

comment:61 Changed 18 months ago by lvh

Documentation of a bunch of class constants and making them private, or alternatively just removing them:

  • RANGE_EXPRESSIONS: made private in r38779 but botched it, fixed in r38781
  • ANGLE_TYPE_NAMES: made private and documented in r38782. There was some NMEA code that relied on this attribute being public, but I removed that dependency.
  • HEMISPHERES_BY_TYPE_AND_SIGN: removed in r38783
  • DOP_EXPRESSIONS: documented and made private in r38777
  • METHOD_PREFIX: removed in r38778 (that code will end up being axed anyway)
  • REQUIRED_CALLBACK_FIELDS: removed in r38776
  • STATEFUL_UPDATE: made private and documented in r38786
  • ACCEPTABLE_UNITS: made private and documented in r38784
  • UNIT_CONVERTERS made private and documented in r38784
  • GSV_KEYS: removed in r38780
  • SPECFIC_SENTENCE_FIXES: made private and documented in r38785

As an extra note: I found some code (NMEA stateful updates) that already relied on Angles being mutable... Perhaps that is the right way forward.

Also, I'm not sure ALLOWED_ATTRIBUTES ought to be private, and it's already documented. If it's private, how do people know which attributes they can set?

Left to document and make private:

  • FIXERS
  • COORDINATE_SIGNS
  • SENTENCE_CONTENTS

And, of course, the remaining TODO:

  • Customization of NMEAProtocol through composition rather than inheritance.
  • Documenting _PositioningSentenceProducerMixin
  • Changing the way NMEAAdapter calls methods
  • PositioningBeaconTests.test_interface should use zope.interface.verify.verifyObject (in addition?). (#6555)
  • Adding Angle types
  • All of the mentioned changes to PositioningBeacon and BeaconInformation
  • Move generic sentence handling code to _sentence module
  • _fixUnits tests

comment:62 Changed 18 months ago by lvh

  • COORDINATE_SIGNS was removed in r38787
  • SENTENCE_CONTENTS was made private and documented in r38788
  • FIXERS was made private and documented in r38789
  • _PositioningSentenceProducerMixin was documented in r38790

Questions:

I was looking into changing how NMEAAdapter calls methods, and I don't think that it will fail trying to call None. Instead, I think it will raise AttributeError. Here's the relevant line:

https://twistedmatrix.com/trac/browser/branches/positioning-3926-2/twisted/positioning/nmea.py#L918

Since that's getattr with two attributes (no default), an exception is raised, which I think is meaningful:

>>> getattr(object(), "xyzzy")
Traceback (most recent call last):
  File "<input>", line 1, in <module>
AttributeError: 'object' object has no attribute 'xyzzy'

Should I still change it?

comment:63 Changed 18 months ago by lvh

  • Since it looks like this won't make the cut for release, @since bumped to 13.2 in r38828.
  • NMEAProtocol customization for specific sentences now happens through a callback passed at initialization, r38829.
  • NMEAProtocol.receiver and .sentenceCallback, both previously public and undocumented, were made private and documented in r38830.

TODO:

  • Adding Angle types
  • API changes PositioningBeacon and BeaconInformation
  • move isUsed to BeaconInformation if possible
  • Move generic sentence handling code to _sentence module
  • _fixUnits tests
  • PositioningBeaconTests.test_interface should use zope.interface.verify.verifyObject (in addition?). (#6555)

comment:64 Changed 18 months ago by lvh

  • BeaconInformation .seen and .used removed in favor of just calling len(); .beacons is now called seenBeacons, both .seenBeacons and .usedBeacons are now sets: r38854
  • verifyObject was added to PositioningBeaconTests.test_interface: r38855
  • Generic sentence handling code moved to _sentence: r38856

TODO:

  • Adding Angle types
  • move isUsed to BeaconInformation if possible
  • _fixUnits tests

comment:65 in reply to: ↑ 60 Changed 17 months ago by tom.prince

Replying to lvh:

I figured out why setSign exists: _angle, the value you'd have to change, is private. This is because the entire object is intended to be immutable. I presume that making it mutable is less ugly than the two alternatives, which are:

  1. Use the _angle implementation detail in the NMEA implementation
  2. Keep setSign.
  1. Create a new Angle.

Also, I just noticed that Heading.setSign only changes the variation, which seems strange and unexpected.

comment:66 Changed 17 months ago by lvh

What does the new Angle type do differently to solve the problem? Besides exposing the value attribute? While I agree it's a bit weird first time you see it, the reason it works that way is because headings don't have signs; but setting the sign of the heading commonly means means setting the sign of its variation :) I'd be amenable to changing it if I can see an obviously better API; right now it seems like that should be "make angle public".

comment:67 Changed 17 months ago by lvh

fixUnits tests were made (more?) complete in r39142.

comment:68 follow-up: Changed 17 months ago by lvh

  • Keywords review added
  • Owner lvh deleted

isUsed was completely removed from beacons and now only exists on beacon information. This was done in revisions r39143, r39179, r39180, r39181.

I'm submitting this back for review: I realize the Angle thing isn't addressed yet, but I don't know what to do with it, and everything else seems to be fixed :)

comment:69 in reply to: ↑ 68 Changed 17 months ago by glyph

Replying to lvh:

I'm submitting this back for review: I realize the Angle thing isn't addressed yet, but I don't know what to do with it, and everything else seems to be fixed :)

Just because the Angle thing isn't perfect doesn't mean it's not good enough to be useful. The purpose of code review on interfaces like this is just to make sure it's good enough that we can live with supporting it as a public API for a while, not that it is perfect and eternal and we can support it unconditionally forever. We can always fix things later, if we have better ideas later.

comment:70 follow-up: Changed 17 months ago by tom.prince

  • Keywords review removed
  • Owner set to lvh
  1. Please add a copyright header:
    1. t.p._sentence
    2. t.p.test.recieiver
  2. @type dataMode: One of L{GPGLLGPRMCFixQualities}.
    • I'd be inclined to spell this @type dataMode: L{GPGLLGPRMCFixQualities}
    • This is slightly inaccurate, since constant containers can't be instantiated. But I think it properly communicates intent. (I guess a ticket should be opened to document the prefered form here. It appears this is the first instance in twisted, so there is no existing practice to consult.)
    • Feel free to disagree.
    • And thinking about it the next, I'm not sure that I agree with myself.
    • Please file a ticket for documenting how to specify twisted.python.constants constants in @type annotations (and, if you think it is appropriate, change the epytext here).
  3. I had a realization of why mutable value objects should be avoided.
    • If somebody keeps a copy of the PositionError given to positionErrorReceived arround, it will get changed from under them when the next sentence comes in.
    • Please change the code to treat all value classes as immutable (bonus points for making them mutable through the public interface).
  4. Regarding angles, my though is, that you go most of the way to having each type of measurement have its own class, except that lattitude and longitude share a class, and that a variation doesn't have its own type. It seems strange to go that far, and no further. I expect either none of the measurements to have their own class, or all of them.
    • Please make each type of measurement consistently have its own type.

Other than the above, this looks good (although I haven't looked at the actual protocol to see if it is implemented correctly). But there is enough code here, and I have looked at it often enough, that I would like another set of eyes on this before it gets merged.

Please resubmit for review after addressing the above points.

comment:71 in reply to: ↑ 70 Changed 14 months ago by lvh

  • Keywords review added
  • Owner lvh deleted

Replying to tom.prince:

  1. Please add a copyright header:
    1. t.p._sentence
    2. t.p.test.recieiver

r40209

  1. @type dataMode: One of L{GPGLLGPRMCFixQualities}.
    • I'd be inclined to spell this @type dataMode: L{GPGLLGPRMCFixQualities}
    • This is slightly inaccurate, since constant containers can't be instantiated. But I think it properly communicates intent. (I guess a ticket should be opened to document the prefered form here. It appears this is the first instance in twisted, so there is no existing practice to consult.)
    • Feel free to disagree.
    • And thinking about it the next, I'm not sure that I agree with myself.
    • Please file a ticket for documenting how to specify twisted.python.constants constants in @type annotations (and, if you think it is appropriate, change the epytext here).

#6777

  1. I had a realization of why mutable value objects should be avoided.
    • If somebody keeps a copy of the PositionError given to positionErrorReceived arround, it will get changed from under them when the next sentence comes in.
    • Please change the code to treat all value classes as immutable (bonus points for making them mutable through the public interface).

I'm confused. I understand the argument for making them immutable, I don't understand why violating that with a public API is still okay?

Also, when you say "make immutable" does that simply mean "treat them as immutable and don't mutate them yourself", or genuinely "make positionError.pdop = 1 raise TypeError".

  1. Regarding angles, my though is, that you go most of the way to having each type of measurement have its own class, except that lattitude and longitude share a class, and that a variation doesn't have its own type. It seems strange to go that far, and no further. I expect either none of the measurements to have their own class, or all of them.
    • Please make each type of measurement consistently have its own type.

I think I can explain that: I built those one at a time, and refactored as I went along, and subclassed when it felt like I had written a lot of that behavior before. I didn't start with "these are all the things I can have, let's write a class for all of them", in which I would certainly have ended up with what you expect.

To me, the class structure I end up with makes a good deal of sense. For example, lats and lons are two coordinates, and they're represented by one class and not two. A variation is just another angle, I don't understand what the benefit is of it having its own type.

Please resubmit for review after addressing the above points.

Thanks for your review!

comment:72 Changed 13 months ago by thijs

  • Description modified (diff)

fyi, #6810 was opened to deprecate t.protocols.gps.rockwell (added reference to ticket description).

comment:73 Changed 13 months ago by lvh

  • Description modified (diff)

Thanks! That's actually one of the (few) remaining reasons to continue using the old thing actually, since t.positioning only supports NMEA, not native Rockwell. I've never encountered a chipset that only supports Rockwell and not (one of the many dialects of) NMEA, but I'm also the only person in the world who tried to install Linux on the one kind of Alpha that didn't have an SRM console, so...

I've also removed the reference to the Github code since that is no longer relevant or what's under review :)

comment:74 Changed 9 months ago by glyph

  • Keywords review removed

All right, that's it, I'm cutting the gordian knot here.

  1. The ValueError raised by Angle.__init__ is not tested.
  2. There are a couple of branch coverage issues in nmea.py.
  3. The docs need to be converted to ReST. Remember that the lore2sphinx buildbot is still online and can help you out with this.
  4. Merge forward so we can be sure Buildbot is happy.
  5. It needs a NEWS file.

At your discretion, also:

  1. Please consider running epywrap over the docstrings in your code so that diffs will be smaller in the future. It looks to me like a bunch of your wrap margins are pretty narrow, the spacing between epytext fields are inconsistent, etc. (My bad that that script isn't in another repo.)
  2. It doesn't really make sense for this to be in Twisted Core, so consider writing its own distutils integration.
  3. Do...something...about python 3. It might be easier to just add this all to the buildbot now rather than have to fix it up later. Your call though.

Modulo any buildbot issues and any other things that you want to address from the mailing list, please consider this approved. The code is clearly up to our quality standards, and it's long past time to get this landed so it can start being maintained instead of being its own sad parallel universe.

comment:75 Changed 9 months ago by glyph

  • Owner set to lvh

comment:76 Changed 9 months ago by hawkowl

  • Author changed from lvh to hawkowl, lvh
  • Branch changed from branches/positioning-3926-2 to branches/positioning-3926-3

(In [41899]) Branching to positioning-3926-3.

comment:77 Changed 9 months ago by hawkowl

  • Owner lvh deleted

comment:78 Changed 9 months ago by hawkowl

  • Keywords review added

In the interests of ninjaing this into 14.0, I have done the following:

  • Merged forward.
  • Ported the documentation to .rst.
  • Updated since 13.2 to since 14.0.
  • Added a topfile.

This branch now merges cleanly with trunk, and I am putting my changes up for review.

comment:79 Changed 8 months ago by lvh

  • Keywords review removed
  • Owner set to lvh

Thank you very much for this! This looks super great :-) Two things remaining from Glyph's review:

  • The ValueError raised by Angle.init is not tested.
  • There are a couple of branch coverage issues in nmea.py.

I'm not entirely sure about the newsfile entry:

twisted.positioning has been added to replace twisted.protocols.gps.

That's the goal, but I think deprecating twisted.protocols.gps is a different ticket. I would like to modify it to make clear that this is an improved API for using positioning systems such as GPS which will eventually supersede twisted.protocols.gps ;-)

comment:80 Changed 8 months ago by lvh

  • Keywords review added
  • Owner lvh deleted

I have re-ran the buildbots. I *think* the remaining branch coverage things constitute bugs in coverage.py in combination with the peephole optimizer, but I'd love for someone to confirm since one of those branch coverage issues actually uncovered a bug ;-)

In the coverage report :

  • L636, which is a dict literal
  • L699, a thoroughly documented bug in the interaction between coverage.py and the peephole optimizer
  • L892, which looks like another impossible result due to for/else.

I introduced some new twistedchecker failures:

C0103:1033,8:NMEAReceiverTest.test_onlyFireWhenCurrentSentenceHasNewInformation: Invalid name "GPGGACallbacks" (should match ([a-z_][a-zA-Z0-9]*)$)
C0103:1045,8:NMEAReceiverTest.test_onlyFireWhenCurrentSentenceHasNewInformation: Invalid name "GPHDTCallbacks" (should match ([a-z_][a-zA-Z0-9]*)$)

I could rename those gpggaCallbacks and gphdtCallbacks, but I really feel this is clearer...

Also another one involving whitespace but I fixed that in [41917].

comment:81 Changed 8 months ago by hawkowl

  • Owner set to hawkowl

comment:82 Changed 8 months ago by hawkowl

  • Keywords review removed
  • Owner changed from hawkowl to lvh

Looks good to me. The TwistedChecker errors are silly, and can be ignored. The API documentation failure seems weird too, since the references are valid. If they're not valid, we can always fix them up later.

Please merge.

comment:83 Changed 8 months ago by lvh

  • Resolution set to fixed
  • Status changed from new to closed

(In [41920]) Merge positioning-3926-3: twisted.positioning, a better positioning framework

Author: lvh, hawkowl
Reviewer: lvh, hawkowl, tom.prince, glyph, exarkun, tazle, dash, therve, ashfall
Fixes: #3926

A better positioning framework. This will replace twisted.protocols.gps.

Note: See TracTickets for help on using tickets.