Skip to content

add docstrings to GPS class attrs #96

New issue

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

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

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Apr 26, 2023
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 54 additions & 18 deletions adafruit_gps.py
Original file line number Diff line number Diff line change
Expand Up @@ -216,50 +216,96 @@ def _parse_data(sentence_type: int, data: List[str]) -> Optional[List]:
return params


# lint warning about too many attributes disabled
# pylint: disable-msg=R0902


# pylint: disable-msg=too-many-instance-attributes
class GPS:
"""GPS parsing module. Can parse simple NMEA data sentences from serial
GPS modules to read latitude, longitude, and more.
"""

# lint warning about too many statements disabled
# pylint: disable-msg=R0915
def __init__(self, uart: UART, debug: bool = False) -> None:
self._uart = uart
# Initialize null starting values for GPS attributes.
self.timestamp_utc = None
"""Timestamp in UTC"""
self.latitude = None
"""Degrees latitude"""
self.latitude_degrees = None
"""Degrees component of latitude measurement"""
self.latitude_minutes = None # Use for full precision minutes
"""Minutes component of latitude measurement"""
self.longitude = None
"""Degrees longitude"""
self.longitude_degrees = None
"""Degrees component of longitude measurement"""
self.longitude_minutes = None # Use for full precision minutes
"""Minutes component of longitude measurement"""
self.fix_quality = 0
"""
GPS quality indicator

| 0 - fix not available
| 1 - GPS fix
| 2 - Differential GPS fix (values above 2 are 2.3 features)
| 3 - PPS fix
| 4 - Real Time Kinematic
| 5 - Float RTK
| 6 - estimated (dead reckoning)
| 7 - Manual input mode
| 8 - Simulation mode
"""
self.fix_quality_3d = 0
"""
The type of fix for a reading

| 1 - no fix
| 2 - 2D fix
| 3 - 3D fix
"""
self.satellites = None
"""The number of satellites in use, 0 - 12"""
self.satellites_prev = None
"""The number of satellites in use from the previous data point, 0 - 12"""
self.horizontal_dilution = None
"""Horizontal dilution of precision (GGA)"""
self.altitude_m = None
"""Antenna altitude relative to mean sea level"""
self.height_geoid = None
"""Geoidal separation relative to WGS 84"""
self.speed_knots = None
"""Ground speed in knots"""
self.track_angle_deg = None
"""Track angle in degrees"""
self._sats = None # Temporary holder for information from GSV messages
self.sats = None # Completed information from GSV messages
self.sats = None
"""Information from GSV messages"""
self.isactivedata = None
self.true_track = None
self.mag_track = None
"""Status Valid(A) or Invalid(V)"""
self.sat_prns = None
"""Satellite pseudorandom noise code"""
self.sel_mode = None
"""
Selection mode

| 'M' - manual
| 'A' - automatic
"""
self.pdop = None
"""Dilution of precision"""
self.hdop = None
"""Horizontal dilution of precision (GSA)"""
self.vdop = None
"""Vertical dilution of precision"""
self.total_mess_num = None
"""Number of messages"""
self.mess_num = None
"""Message number"""
self._raw_sentence = None
self._mode_indicator = None
self._magnetic_variation = None
self.debug = debug
"""Toggles debug mode. When True, prints the incoming data sentence to the console"""

def update(self) -> bool:
"""Check for updated data from the GPS module and process it
Expand Down Expand Up @@ -535,15 +581,6 @@ def _parse_gga(self, data: List[str]) -> bool:
self.longitude_degrees, self.longitude_minutes = _read_int_degrees(data, 3, "w")

# GPS quality indicator
# 0 - fix not available,
# 1 - GPS fix,
# 2 - Differential GPS fix (values above 2 are 2.3 features)
# 3 - PPS fix
# 4 - Real Time Kinematic
# 5 - Float RTK
# 6 - estimated (dead reckoning)
# 7 - Manual input mode
# 8 - Simulation mode
self.fix_quality = data[5]

# Number of satellites in use, 0 - 12
Expand Down Expand Up @@ -662,8 +699,7 @@ def _parse_gsv(self, talker: bytes, data: List[str]) -> bool:
# been seen for 30 seconds
timestamp = time.monotonic()
old = []
for i in self.sats:
sat = self.sats[i]
for sat in self.sats.items():
if (timestamp - sat[4]) > 30:
Comment on lines -666 to 703
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bug! .items() always returns a tuple[k, v] (length 2) so sat[4] will always raise an IndexError, no matter what!

The tests would have caught this, if they were run. 2f6015e for some reason decided to not run them in CI anymore, so regressions like this are likely and will make it into the next release.

(venv) PS C:\Users\jonas\Downloads\Adafruit_CircuitPython_GPS> pytest
================================================= test session starts =================================================
platform win32 -- Python 3.11.1, pytest-7.3.1, pluggy-1.0.0
rootdir: C:\Users\jonas\Downloads\Adafruit_CircuitPython_GPS
collected 48 items

tests\adafruit_gps_test.py ...............................................F                                      [100%]

====================================================== FAILURES =======================================================
________________________________ test_GPS_update_from_GSV_both_parts_sats_are_removed _________________________________

    def test_GPS_update_from_GSV_both_parts_sats_are_removed():
        gps = GPS(uart=UartMock())
        with mock.patch.object(GPS, "readline") as m:
            with freeze_time("2021-10-20 19:00:00"):
                # first part of the request
                m.return_value = b"$GPGSV,2,1,04,01,40,083,46,02,17,308,41*78\r\n"
                assert gps.update()
                assert gps.total_mess_num == 2
                assert gps.mess_num == 1
                assert gps.satellites == 4
                # first time we received satellites, so this must be None
                assert gps.sats is None
            # some time has passed so the first two satellites will be too old, but
            # this one not
            with freeze_time("2021-10-20 19:00:20"):
                # second part of the request
                m.return_value = b"$GPGSV,2,2,04,12,07,344,39,14,22,228,45*7c\r\n"
                assert gps.update()
                assert gps.total_mess_num == 2
                assert gps.mess_num == 2
                assert gps.satellites == 4
                # we should now have 4 satellites from the two part request
                assert set(gps.sats.keys()) == {"GP1", "GP2", "GP12", "GP14"}

            # time passed (more than 30 seconds) and the next request does not
            # contain the previously seen satellites but two new ones
            with freeze_time("2021-10-20 19:00:31"):
                # a third, one part request
                m.return_value = b"$GPGSV,1,1,02,13,07,344,39,15,22,228,45*7a\r\n"
>               assert gps.update()

tests\adafruit_gps_test.py:467:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
adafruit_gps.py:354: in update
    result = self._parse_gsv(talker, args)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <adafruit_gps.GPS object at 0x0000014E020ECB90>, talker = 'GP', data = [1, 1, 2, 13, 7, 344, ...]

    def _parse_gsv(self, talker: bytes, data: List[str]) -> bool:
        # GSV - Satellites in view
        # pylint: disable=too-many-branches

        if data is None or len(data) not in (7, 11, 15, 19):
            return False  # Unexpected number of params.
        data = _parse_data(
            {7: _GSV7, 11: _GSV11, 15: _GSV15, 19: _GSV19}[len(data)],
            data,
        )
        if data is None:
            return False  # Params didn't parse

        talker = str(talker, "ascii")

        # Number of messages
        self.total_mess_num = data[0]
        # Message number
        self.mess_num = data[1]
        # Number of satellites in view
        self.satellites = data[2]

        sat_tup = data[3:]

        satlist = []
        timestamp = time.monotonic()
        for i in range(len(sat_tup) // 4):
            j = i * 4
            value = (
                # Satellite number
                "{}{}".format(talker, sat_tup[0 + j]),
                # Elevation in degrees
                sat_tup[1 + j],
                # Azimuth in degrees
                sat_tup[2 + j],
                # signal-to-noise ratio in dB
                sat_tup[3 + j],
                # Timestamp
                timestamp,
            )
            satlist.append(value)

        if self._sats is None:
            self._sats = []
        for value in satlist:
            self._sats.append(value)

        if self.mess_num == self.total_mess_num:
            # Last part of GSV message
            if len(self._sats) == self.satellites:
                # Transfer received satellites to self.sats
                if self.sats is None:
                    self.sats = {}
                else:
                    # Remove all satellites which haven't
                    # been seen for 30 seconds
                    timestamp = time.monotonic()
                    old = []
                    for sat in self.sats.items():
>                       if (timestamp - sat[4]) > 30:
E                       IndexError: tuple index out of range

adafruit_gps.py:703: IndexError
=============================================== short test summary info ===============================================
FAILED tests/adafruit_gps_test.py::test_GPS_update_from_GSV_both_parts_sats_are_removed - IndexError: tuple index out of range
============================================ 1 failed, 47 passed in 0.26s =============================================
(venv) PS C:\Users\jonas\Downloads\Adafruit_CircuitPython_GPS>

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird, pre-commit claims to have run the tests according to the printout from the composite action... it says "Passed" and not "Skipped".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do remember running precommit before merging this, but I don't recall what jobs it ran. Let me dig back in...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nvm, it was actually removed in: cca0f0f.

pre-commit has nothing to do with running unit tests! if mypy was setup via pre-commit, it would have caught it.

error: Tuple index out of range  [misc]

This part or something similar is needed to run the tests.

- name: run tests
run: |
pip install --upgrade tox
tox -e py

Copy link
Member

@tekktrik tekktrik May 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm big dumb, I'm thinking of pylint. Yeah, currently pytest isn't run though it didn't look like it was set to run before, for what it's worth. Either way, an oversight on the part of the CI, whoopsies!

I'll create an issue for fixing the libraries in general. Good catch!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah ok. I just pushed a new PR, but it's tiny and I don't mind if you ignore it in favor of yours

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, no, let's just add it to yours! Just take all the packages listed in requirements-dev.txt and move them to optional_requirements.txt. Then we can delete the unnecessary requirements-dev.txt.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. do I need to maintain the attribution from requirements-dev.txt?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A good point! You can add another SPDX line for that author in the merged file so both are retained.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool, pushed an update with those changes as well

old.append(i)
for i in old:
Expand Down