Open-Agriculture / AgIsoStack-plus-plus

AgIsoStack++ is the completely free open-source C++ ISOBUS library for everyone
https://agisostack.com/
MIT License
168 stars 40 forks source link

[TC]: Add ISO11783-11 database lookup functionality #413

Closed ad3154 closed 5 months ago

ad3154 commented 5 months ago

Describe your changes

@GwnDaan I'd like your help assessing the format/schema of this "database". I currently have it implemented as a dumb C style array because that seemed to be the most non-c++14+ way of ensuring that on microcontrollers the data ends up in flash instead of in RAM.

How has this been tested?

Added unit tests to validate a couple random DDIs return expected values, plus one undefined DDI returns a known, default value.

You can run the code generation yourself by running the included python script. python3 code_gen_ddi_database.py

GwnDaan commented 5 months ago

I currently have it implemented as a dumb C style array

I really like this "dumb" way, no need to overcomplicate things there. Though I do think there is a few things we could improve upon:

  1. Instead of replacing the entire file with the python script, we can use a bit of regex and replace just the parts we're interested in (for inspiration). This will save us some confusion with having the file duplicated in two places, and allows us to more easily modify it in a conventional way
  2. Generating an enum for the DDI name mapping to the index number. I imagine we should be interested in the meaning of the DDI rather than the number behind it
  3. Generating an enum for the units, as comparing a string can be more user-error prone than comparing it with another enum
ad3154 commented 5 months ago

we can use a bit of regex

That could make sense, as long as it also adds in new ones as well, though I would like to let someone else tackle that if possible, I despise regex, and I'm not sure if matching on any part of a definition would result in a perfect result depending on what they change.

Generating an enum for the DDI name mapping to the index number. I imagine we should be interested in the meaning of the DDI rather than the number behind it

So, I think it depends on if you're a server or a client, so we may want both. Servers will care about the DDI's number, not the name. Clients will care about the name, not the number, generally. I think it probably makes sense to add this enum based approach additionally I guess.

Generating an enum for the units, as comparing a string can be more user-error prone than comparing it with another enum

I think the use case is again maybe a bit different based on different applications' requirements. I think if you want to display the units, having the string is nice, but if you want to process the DDI in some way, you'll want the enum to know what to do with it in an efficient way. So I think once again, maybe having both is ideal

GwnDaan commented 5 months ago

I would like to let someone else tackle that if possible

No worries, I proposed the changes in #415. Seems to be working quite well as long as the file is in a valid state.

So I think once again, maybe having both is ideal

I agree, including both seems to be the best of both worlds here for the reasons you provided

sonarcloud[bot] commented 5 months ago

Quality Gate Passed Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

3 New issues
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud