AprilRobotics / apriltag

AprilTag is a visual fiducial system popular for robotics research.
https://april.eecs.umich.edu/software/apriltag
Other
1.56k stars 535 forks source link

Replaced calls to exit() #224

Closed j-medland closed 2 years ago

j-medland commented 2 years ago

Here is a belated PR for issue #182.

I have removed all the calls to exit() that might be encountered and set the errno where it isn't already set by the operation that failed.

As the user might not be checking the errno values I thought it best to avoid their application crashing by returning an empty detections array, where relevant, to avoid them encountering a null-pointer.

Any feedback is welcome and I am happy to tweak as required.

j-medland commented 2 years ago

I am happy to remove the style changes. I thought I had caught them all prior to submitting the PR.

With regards to printing messages to the console - An alternative to using printf() would be good.

What error strategy would you prefer?

  1. Just use errno
  2. Write all information to stderr using perror or some debug-print macro that is disabled when building the library for release
  3. Same as 2 but write to a text-file.
  4. Have some global error store that stores the previous 1/5/10/n errors as strings with some API calls to query it.
  5. Something else

Cheers

christian-rauch commented 2 years ago

We should definitely use the errno (1) and optionally perror (2) on the errno that are already set by the functions we call and those that we set ourself in the apriltag library. perror cannot be used alone.

I would not implement a complex logging system that writes to a log file or global storage.

j-medland commented 2 years ago

I have made some updates which try to unify the printing of debug messages with the addition of a debug_print macro which will be removed in release builds and added in error checking to the examples in the relevant locations.

For Example:

$ apriltag_demo -a 3 -f tagStandard41h12 tag.jpg
apriltag.c:224:quick_decode_init(): Failed to allocate hamming decode table # <-- this is the debug message
Unable to add family to detector due to insufficient memory to allocate the tag-family decoder. Try reducing "hamming" from 3 or choose an alternative tag family. # <-- this is printed in the demo code

I have then tried to update all the sections of the main library that would print debug messages to use this new macro.

The areas of code that were from some previous development/debugging that had exit or print statements have been enclosed in compiler removed #if 0/#endif sections although I guess they could just be commented out or simply removed. I think this makes more sense then trying to wrap them into asserts given they currently aren't even active in the debug build but I can try and change this if you still prefer the assert.