migrate to gpiod

[CullenSharp]

Integrates gpiod helper module, adds commentary to skytraq module, and refactors package entry point (__main__, __init__)

[CullenSharp]

I got gpiod to work... I think. at least that's what I'm seeing from my remote lab. I need to double check this when I'm in person, but I don't seem to be getting any messages from the orion.

[CullenSharp]

Progress! I verified that UART2 was connected to ttyS2 with dmesg, and changed the baud to 115200 as described in the datasheet for the Orion B16. After asserting the GPS enable pin and writing the binary mode string (BINARY_MODE = b"\xa0\xa1\x00\x03\x09\x02\x00\x0b\x0d\x0a"), I get small bursts of data on the UART. Not really sure what this data is.

Edit: after reviewing the NMEA 183 standard a little, I have decided that this is mostly gibberish.

>>> ser.write(binary)
10
>>> ser.readline()
b'\x0b\x03\x03\t\xfe'
>>> ser.write(binary)
10
>>> ser.readline()
b'\t\x02\x00\x0b\r\xfa'
>>> ser.readline()
b''
>>> ser.write(binary)
10
>>> ser.readline()
b'H\x00\x0b\r\xfa'

[CullenSharp]

I was just thinking; it would be good to defensively check if data is available in the input buffer before attempting a readline (e.g. ser.in_waiting).

[ThirteenFish]

I like that most of the body has been moved out of __main__.py, this file should be a super thin wrapper that enables the thing that it does. To that end we can reduce it even more, this code largely repeats what the python interpreter already does for us: when exiting it will return 0 or on exception will return 1 and print the stack trace to stderr.

The only difference here is that we suppress the stack trace on ValueError and OSError. Given that we never normally want to exit exceptionally we really do want the stack trace in all cases (except possibly KeyboardInterrupt).

This file should be:

from . import main
main()

While I'm here the docstring could probably be refined as well. Typically __main__.py is so small and obvious that it doesn't get a docstring but I know linters complain about it so it doesn't hurt to have something small. If we are to add something then this isn't really the main entry point, this file only executes when executing this module as a script - as in python -m oresat_gps. This really only happens during testing, the primary execution path is through the oresat-gps script generated by the pyproject.toml [project.scripts] section.

Something like """Module entry point.""" would be more accurate but it's also mostly useless because we already know what __main__.py is for and if we didn't it provides no actionable information. So something like Module entry point. Invoke with 'python -m oresat_gps'. is both terse and potentially useful.

[CullenSharp]

I'm following. This should be an easy fix.

[CullenSharp]

This really only happens during testing,

This is a finer detail I'd like clarification on. When running unittest, is each module run directly (e.g. __name__ = __main__ instead of module_name)?

[ThirteenFish]

By testing here I mean like making a change and then running the code by hand. Running it as a module out of the source directory ensures you pick up the changes you made and aren't getting a stale installed version.

On unit tests no, only the entry point of the test harness will get "__main__", so like the initial file of pytest or unittest.

[ThirteenFish]

It doesn't really make sense to have if __name__ == "__main__": in __init__.py, we would never invoke this file directly (i.e. we'd have to python oresat_gps/__init__.py for this condition to be true). We'd either do python -m oresat_gps or call the oresat-gps script generated in the pyproject.toml. This can just be deleted.

[ThirteenFish]

This was correct as it was before because the pyproject.toml makes use of [dependency-groups] instead of [project.optional-dependencies].

The [dependency-groups] section doesn't get included in the produced package metadata so it's great for things that users would never want to install like development tools. On the other hand [project.optional-dependencies] do get included so they're best used for things that a user might want but aren't critical, like utility scripts for something that's otherwise a library.

[ThirteenFish]

The GPIO lines should never be None. If we don't find the line we wanted an exception should be thrown, we want to know as soon as possible that something is wrong. There should never be a situation in which we try to operate the GPS unit without its associated GPIO lines.

Additionally I'm not sure that the semantics here are correct. In disconnect() we want to actively assert that the lines are low, not return them to default values which may not be the value we were expecting. connect() isn't a constructor and disconnect() isn't a destructor, the class should find the relevant lines once at initialization and maintain an open reference to them for the lifetime of the resulting object.

[CullenSharp]

This is a bigger issue I think. In gps_service.py we have this.

def on_loop_error(self, error: str) -> None:
    self._skytraq_power_off()
    self._state = GpsState.ERROR
    logger.exception(error)

This is correct in that it doesn't kill the service, but it releases the gpio lines prematurely. This leads to a re-release when the program terminates.

As for the logic level, this (to my knowledge) is handled in the device tree. PIN_OUTPUT_PULLDOWN should assert that the pin is a logic low when not held. Anecdotally, this aligns with what I have observed measuring the logic levels in the lab.

  gps_gpio3_pins: pinmux-gps-gpio3-pins {
    pinctrl-single,pins = <
      //...
      /* GPS_EN toggles power for the gps section of the card */
      AM33XX_IOPAD(AM335X_PIN_MII1_RX_DV, PIN_OUTPUT_PULLDOWN | MUX_MODE7)
    >;
  };

[ThirteenFish]

As mentioned before I'd very much like to never iterate over all available lines, instead we should supply:

• chip
• line offset
• line name

and then open the line by the numbers and verify the name is correct. Now that the device trees can guarantee a stable chip ordering this should be fine to do.

This has the effect of both both a minimal startup and as a double check that configurations agree between the application and OS.

[ThirteenFish]

I'm not sure I see the value of saving the config information here in a dataclass if we're going to then going to discard it and operate only on the LineRequest. request() could be a standalone function that takes what it needs and returns the allocated resource. After getting the line it should then also check that the name, direction, and initial value are all correct. We can make some simplifying assumptions in that the GPS application only wants outputs and only wants initially inactive lines. Users will never have to configure those things.

Also the consumer field should be something related to the process and the same across all lines allocated to this process so it can be hardcoded in.

If you do want a simple wrapper for the GPIO lines take a look at what LineOut does in uniclogs-stationd. It's not exactly what we want here but it does aim to be as thin as is reasonable.

[CullenSharp]

This makes sense. We don't really need fine-grained control over the line, since it's just set and forget.

Incidentally, I could just add this function as a static method of the skytraq class. Would that be preferable?

edit: Having it in a separate module is kind of nice from a testing perspective. On the bench, I've been using the _gpio module to enable the GPS without the overhead from the skytraq class. This seems a little silly, but maybe the skytraq class can hold a reference to the function in _gpio (e.g. skytraq.request = _gpio.request) ? That way the separation is maintained.

[ThirteenFish]

Keeping it in a separate module seems like a decent idea, even if it's small. Eventually we'll also want to add gpio-sim stuff so it'll grow again.

[ThirteenFish]

This new description focuses a lot more on what ends up being implementation details. As a user of the project there's nothing I can do with the fact that we talk over a UART to the B16 in a mix of NEMA and custom binary or that we have a driver. The bulk of what this project aims to do is actually to hide that part entirely.

The introductory description should focus more on things for users than things for maintainers so:

• It's a GPS.
• Mentioning it requires custom hardware is good (even better would be a link to the hardware repo).
• Mentioning the MAX and the Skytraq are interesting but only in the sense that we have two reception methods - an SDR (not yet implemented) and a COTS receiver.
• Talking more about how we would see the output from GPS would be great, so a little more about CANopen and the specific data that is provided over that interface. Position, velocity, time, etc. Maybe the tools one could use to see this output (as a side note I'd like to eventually write a util that lives in this repo that makes it easy to access that data).
• Speaking of time, people typically don't realize GPS is a clock, so a brief note on why we get time from GPS and its intent for use in the wider system.
• Speaking of the wider system, why do we even have a GPS in a satellite to begin with? There are no roads in space, what then consumes this data?
• OLAF is useful to link to because that has some user facing controls, especially the debugging webpage so a note on how to access that would be helpful.

The implementation details and links could be moved into the code itself, or at most an Architecture section down at the end of the README.

[CullenSharp]

I see. I'm not used to differentiating between end users and maintainers. I only added what I wish I had known from the beginning, but this will inevitably go out of sync when the implementation changes.