Hacker News Reader

Comment by nanis | Hacker News Reader

Ouch. The method is called `update` and it is littered with stuff like:

   if (frame % 10) == 0:

      can_sends.append(make_can_msg(1648, b'\x00\x00\x00\x40\x00\x00\x50\x00', 1))
      can_sends.append(make_can_msg(1649, b'\x10\x10\xf1\x70\x04\x00\x00\x00', 1))

      ...

I know the identifiers etc are probably hard to catalog, but that is all the more reason to give things symbolic names and maybe even avoid having to specify those IDs repeatedly.

View on HN

See my other comment about Ford code being reference only.

Supported cars have most signals defined in a dbc file, the mapping from name to id and bytes. See https://github.com/commaai/opendbc

This results in much cleaner car abstraction layers: https://github.com/commaai/openpilot/blob/de0ce142ae51cf9c85...

Thanks for the reference! That certainly does look much nicer.

That being said, I do still see some "magical" looking constants and dictionary keys here[1], as well as lots of small numbers later in the file. I think it would be confidence-inspiring to have those better documented as well.

Edit: I found some more magic-looking bytestrings in the Volkswagen support here[2].

[1]: https://github.com/commaai/openpilot/blob/de0ce142ae51cf9c85...

[2]: https://github.com/commaai/openpilot/blob/de0ce142ae51cf9c85...

At some point the values have no deeper meaning and are just whatever VW decided on as their API. DBC files have concepts of enums, but that’s really diminishing returns and wouldn’t make the code that much easier to understand.

Also sometimes there are fixed values in the message that we’ve only observed as static data. Ironically that makes it impossible to reverse engineer what they mean. So those just have to be labeled as hardcoded value.

Those are actually magic. Those are the ECU firmware identifiers from the manufacturer that are used by openpilot to recognize which car it’s connected to.

Reply on news.ycombinator.com