luxonis / depthai-core

DepthAI C++ Library
MIT License
234 stars 126 forks source link

`Device::getDeviceName()` has unpredictable name strings #863

Open diablodale opened 1 year ago

diablodale commented 1 year ago

Multiple OAK retail models owned by me and my customers are returning unpredictable name strings with Device::getDeviceName(). In addition, the name string unpredictably has boolean flags encoded in it. This is inconsistent and will require unmanagable logic chains in customer code to reverse engineer correct model discovery info.

Directly related to 2021 issue https://github.com/luxonis/depthai-core/issues/285

Setup

Repro

  1. build a c++ app that calls device.getDeviceName(), device.readCalibration().getEepromData() and inspects the results
  2. attach multiple retail OAK devices and use that API to get and examine all their name strings
  3. run the app and inspect

Result

deviceName strings are unpredictable and not consistent. They are not useable for identifying existing and forthcoming models. I have already asked for a list of the name strings here https://github.com/luxonis/depthai-core/issues/285#issuecomment-1609461308 for existing models -- though this type of list is immediately outdated when a new OAK model is produced.

One root issue is boolean flags should not be encoded in a text string. They should instead be boolean flags in a struct. As demonstrated by this issue, string mangling is unreliable and already errant. This is similar to the other issue of encoding "NONE" to mean no IMU.

Naturally, there is an ever growing list of retail models so I can not list all the errant ones today. Below, I list the errant ones that I and customers have seen so far.

physical OAK model EepromData:: productName EepromData:: boardName Device:: getDeviceName() CameraFeatures:: hasAutofocus Notes
oak d pro poe auto focus (pre-production silver) "" "OAK-D-PRO-POE" "OAK-D-PRO-POE" true no text flag for focus, incomplete text flag for connectivity (missing usb)
oak d lite fixed focus (edition w/o short risk) "" "OAK-D-LITE" "OAK-D-LITE" true❌ no text flag for focus or connectivity
oak d lite fixed focus (edition with short risk) "" "OAK-D-LITE" "OAK-D-LITE" true❌ no text flag for focus or connectivity
oak d (production first batch) "" "OAK-D" "OAK-D" true no text flag for focus or connectivity
oak d pro auto focus (sku: A00546) (n/a from customer) (n/a from customer) "OAK-D-PRO-AF"❌ (n/a from customer) includes text flag for focus, no text flag for connectivity

Expected

There should be no POE flag, no AF flag, no FF substring flag, etc. Each of those boolean/enum should be in a struct. String parsing for these flags is error-prone for Luxonis (see 4 bugs I've reported this week) and difficult+error-prone for customers. Rules for these string parsers will be complex and fragile with the combinations of existing and new models. For example: `if oak-d then assume autofocus, else if oak-d-lite then ignore hasAutofocus and use position==0, else if oak-d-pro then look for "AF" somewhere in the string, but if it is poe then look for "AF" or "POE" in some order in the string, which models use "AF", which models use "only FF", which models no nothing and we have to maintain each our own lookup lists (like OAK-D), etc. 😝

Luxonis attempted to encode boolean flags/enums in a text string. πŸ‘ŽI suspect there are even more like wide-angle, tof, etc. If Luxonis chooses to not provide model discovery data in a struct, and instead wants to encode it in a DASH-SEPARATED-TEXT-STRING, then I request Luxonis document exactly the text coding that will now and forever be used so that every customer can write each their own text string->struct parser (which is redudant error-prone and soon outdated code maintained by every customer πŸ€¦β€β™€οΈ).

Instead of mangling a text string and scraping it later with a custom parser, below is my preferred expected result. CONN_IP describes the IP connectivity. The data connectivity is not "PoE"...that is how a device gets electricity. This is just an example. Naturally, a holistic approach should actually be used so that discovery of time-of-flight, deep ir, wide angle, long distance, etc. is successful. These are... in modern lingo... "tags". Similar to how social media posts have tags. Tagging is trivial to implement with json and std::unordered_set. This could also be done in a string with any delimiter.

physical OAK model EepromData:: productName EepromData:: boardName Device:: getDeviceName() CameraFeatures:: hasAutofocus new connectivity flagField
oak d pro poe auto focus (pre-production silver) "SKU_xxxx" "BWxxxxx" "OAK-D-PRO" true CONN_IP \| CONN_USB
oak d lite fixed focus (edition w/o short risk) "SKU_xxxx" "BWxxxxx" "OAK-D-LITE" false CONN_USB
oak d lite fixed focus (edition with short risk) "SKU_xxxx" "BWxxxxx" "OAK-D-LITE" false CONN_USB
oak d (production first batch) "SKU_xxxx" "BW1098OBC" "OAK-D" true CONN_USB
oak d pro auto focus (sku: A00546) "SKU_A00546" "BWxxxxx" "OAK-D-PRO" true CONN_USB
themarpe commented 1 year ago

Thanks for the report @diablodale

getDeviceName is more inline with SKU than it is for detecting features of a device. PoE & USB variants differ to the extent that they are marked separately. The connectivity itself however is denoted by protocol that is identified when discovering the devices (DeviceInfo).

The hasAutofocus and IMU reporting is indeed errorenous and we will look out to address that.

though this type of list is immediately outdated when a new OAK model is produced

WRT the known models of devices out there, this could be referenced: https://github.com/luxonis/depthai-boards

diablodale commented 1 year ago

This continues to be unclear to me.

For now, I've written custom code (like I warned above) to scrape and probe and hack to understand a OAK device's capabilities. For example, I'm still hacking focus using fixedFocus = eeprom.lensPosition == 0 since there is no other way to determine focus as per https://github.com/luxonis/depthai-core/issues/326#issuecomment-1011815563

themarpe commented 1 year ago

I agree on the take - there is a bit overlap between SKU and DeviceName in our case at the moment. Things like AF/FF should be left under camera features, though connectivity (while it could have its own "features" field) is currently part of Device Name as OAK-D-S2 vs OAK-D-S2-POE are "different devices". (Same goes for Wide devices, but not their variants, etc...)

Main improvements from this are:

AF/FF detection capabilities improvements WIP in other issue.