serial_link.py -> swiftnav.sbp.client

[mfine]

Dumping this here for now to collect feedback - this does not reflect its final resting place or integration into libsbp. Was going to open this under piksi_firmware, but the changes are probably too confusing to be helpful to compare. But please do compare this next to the existing serial_link.py.

Things are roughly in place but not completely polished. The tests are intermixed with the code for convenience. There are a number of TODOs sprinkled throughout. And some of the existing code functionality has not been brought over. But generally, some goals here were:

• fix bugs
• clean up the structure and avoid coupling - keep to well-defined objects
• add tests
• add context managers to better handle resources
• more straight-line code, less branchy logic
• capture more commonality
• easier extensibility
• better documentation

Feel free to pile on comments, critiques, more idiomatic patterns, etc.

/cc @fnoble @mookerji

[fnoble]

Could use readall here too?

[mfine]

I used readall there initially, but it undo's some logic of the original serial_link - if wants_to_stop is set and nothing is returned from the handle serail_link.py#L177-178. Happy to have it block waiting for input, though - would be more uniform.

[fnoble]

Looks really good, much cleaner and more logical.

General thoughts (I'm sure you are already thinking about this):

• Should be split into several files, I'd suggest separating out the Loggers, the different Readers and the tests(?) I imagine serial_link.py will eventually just be a simple example of choosing a reader and a logger and gluing them together.
• When you add write support it might be easier to make it part of the current Reader class to make dealing with the file handle easier?

[mfine]

Thanks for the review and comments - all stuff I had been thinking about. Will definitely move towards a general Driver class, and splitting things up into files (as well as tests directory and test files).

Still kind a little unsure about the degree of inheritance used here vs composition. Think some of the functionality could be better represented by helpers that get composed vs. inherited, e.g., a LogHelper or something to cover all the timestamp and time diff.

Will incorporate the feedback and update.

[fnoble]

Cool. The use of inheritance here seems pretty natural and idiomatic to me but I'd be curious to chat tomorrow about what the compositional approach would look like.

On a related note, one thing I have seen before is that although python doesn't really have a way of defining abstract interfaces you can make the interface clear by adding the methods you want to the base class and having them raise NotImplementedError. That way its clear what needs to be implemented to conform to the interface. For example you could have:

class Logger(object):

  ...

  def __call__(self, msg):
    """
    Logger instances are simply called with a message to log
    """
    raise NotImplementedError()

[mookerji]

Looking at this now. This has a great commit message, btw.

[mookerji]

Yep: this is going to live with the SBP definition, or a separate CRC/error correction module. See: https://github.com/swift-nav/libsbp/blob/master/python/libsbp/sbp.py

[mfine]

I had come up with a stateful CRC16 object that might result in some more conducive usage - don't think it makes much of an improvement, but it does capture the "accumulating" notion of the CRC which is more explicit with the functional approach here:

class CRC16(object):
  """
  CRC16 implementation acording to CCITT standards.
  """
  TAB = [
    0x0000,0x1021,0x2042,0x3063,0x4084,0x50a5,0x60c6,0x70e7,
    0x8108,0x9129,0xa14a,0xb16b,0xc18c,0xd1ad,0xe1ce,0xf1ef,
    0x1231,0x0210,0x3273,0x2252,0x52b5,0x4294,0x72f7,0x62d6,
    0x9339,0x8318,0xb37b,0xa35a,0xd3bd,0xc39c,0xf3ff,0xe3de,
    0x2462,0x3443,0x0420,0x1401,0x64e6,0x74c7,0x44a4,0x5485,
    0xa56a,0xb54b,0x8528,0x9509,0xe5ee,0xf5cf,0xc5ac,0xd58d,
    0x3653,0x2672,0x1611,0x0630,0x76d7,0x66f6,0x5695,0x46b4,
    0xb75b,0xa77a,0x9719,0x8738,0xf7df,0xe7fe,0xd79d,0xc7bc,
    0x48c4,0x58e5,0x6886,0x78a7,0x0840,0x1861,0x2802,0x3823,
    0xc9cc,0xd9ed,0xe98e,0xf9af,0x8948,0x9969,0xa90a,0xb92b,
    0x5af5,0x4ad4,0x7ab7,0x6a96,0x1a71,0x0a50,0x3a33,0x2a12,
    0xdbfd,0xcbdc,0xfbbf,0xeb9e,0x9b79,0x8b58,0xbb3b,0xab1a,
    0x6ca6,0x7c87,0x4ce4,0x5cc5,0x2c22,0x3c03,0x0c60,0x1c41,
    0xedae,0xfd8f,0xcdec,0xddcd,0xad2a,0xbd0b,0x8d68,0x9d49,
    0x7e97,0x6eb6,0x5ed5,0x4ef4,0x3e13,0x2e32,0x1e51,0x0e70,
    0xff9f,0xefbe,0xdfdd,0xcffc,0xbf1b,0xaf3a,0x9f59,0x8f78,
    0x9188,0x81a9,0xb1ca,0xa1eb,0xd10c,0xc12d,0xf14e,0xe16f,
    0x1080,0x00a1,0x30c2,0x20e3,0x5004,0x4025,0x7046,0x6067,
    0x83b9,0x9398,0xa3fb,0xb3da,0xc33d,0xd31c,0xe37f,0xf35e,
    0x02b1,0x1290,0x22f3,0x32d2,0x4235,0x5214,0x6277,0x7256,
    0xb5ea,0xa5cb,0x95a8,0x8589,0xf56e,0xe54f,0xd52c,0xc50d,
    0x34e2,0x24c3,0x14a0,0x0481,0x7466,0x6447,0x5424,0x4405,
    0xa7db,0xb7fa,0x8799,0x97b8,0xe75f,0xf77e,0xc71d,0xd73c,
    0x26d3,0x36f2,0x0691,0x16b0,0x6657,0x7676,0x4615,0x5634,
    0xd94c,0xc96d,0xf90e,0xe92f,0x99c8,0x89e9,0xb98a,0xa9ab,
    0x5844,0x4865,0x7806,0x6827,0x18c0,0x08e1,0x3882,0x28a3,
    0xcb7d,0xdb5c,0xeb3f,0xfb1e,0x8bf9,0x9bd8,0xabbb,0xbb9a,
    0x4a75,0x5a54,0x6a37,0x7a16,0x0af1,0x1ad0,0x2ab3,0x3a92,
    0xfd2e,0xed0f,0xdd6c,0xcd4d,0xbdaa,0xad8b,0x9de8,0x8dc9,
    0x7c26,0x6c07,0x5c64,0x4c45,0x3ca2,0x2c83,0x1ce0,0x0cc1,
    0xef1f,0xff3e,0xcf5d,0xdf7c,0xaf9b,0xbfba,0x8fd9,0x9ff8,
    0x6e17,0x7e36,0x4e55,0x5e74,0x2e93,0x3eb2,0x0ed1,0x1ef0
  ]

  def __init__(self):
    self.crc = 0

  def call(self, s):
    for ch in s:
      self.crc = (((self.crc << 8) & 0xFFFF) ^ CRC16_TAB[((self.crc >> 8) & 0xFF) ^ (ord(ch) & 0xFF)]) & 0xFFFF
    return self.crc

  def __call__(self, s):
    return self.call(s)

With usage looking like:

>>> crc = CRC16()
>>> crc("123")
38738
>>> crc("123")
550
>>> crc("123")
21874
>>> 

[fnoble]

The class interface good, the old function was basically just copied directly from C without much thought

[mookerji]

Oh man, thanks for doing this. This is going to be exceptionally maintainable/approachable going forward.

[mfine]

I laid down an initial directory structure, but I really don't know how python modules path'ing works. :sadpanda:

[fnoble]

New file / module / class hierarchy looks great.

[mookerji]

... so I think this is ready to merge after porting some of the remaining features already listed in the TODO file (serial/device exceptions, listing existing devices)?

[mfine]

Yeah, that's my plan - once the generated code comes in and I can remove the todo.py, I'll bring this in.

[mfine]

Commit squashed, ready to merge.