[RST-2432] Skip transactions on a per-sensor basis #102
Conversation
| @@ -39,7 +39,6 @@ | |||
| #include <fuse_optimizers/fixed_lag_smoother_params.h> | |||
| #include <fuse_optimizers/optimizer.h> | |||
| #include <fuse_optimizers/variable_stamp_index.h> | |||
| #include <fuse_optimizers/vector_queue.h> | |||
There was a problem hiding this comment.
The VectorQueue class was a wrapper around a std::vector that provided push() and pop() methods for the front and back. Now that I need to process things in the middle of the vector, the number of functions that had to be exposed from the std::vector to the VectorQueue was getting ridiculous. I refactored, removing this class and just using the std::vector directly.
| // Processing was successful. Add the results to the final transaction, delete this one, and move to the next. | ||
| transaction.merge(*element.transaction, true); | ||
| ++transaction_iter; | ||
| pending_transactions_.erase(transaction_iter.base()); // Reverse iterators are weird |
There was a problem hiding this comment.
Using reverse iterators is weird.
https://stackoverflow.com/questions/1830158/how-to-call-erase-with-a-reverse-iterator
| // Processing was successful. Add the results to the final transaction, delete this one, and move to the next. | ||
| transaction.merge(*element.transaction, true); | ||
| ++transaction_iter; | ||
| pending_transactions_.erase(transaction_iter.base()); // Reverse iterators are weird |
| (current_time - element.transaction->stamp()) << " seconds, which is greater " << | ||
| "than the 'transaction_timeout' value of " << params_.transaction_timeout << | ||
| ". Ignoring this transaction."); | ||
| ++transaction_iter; |
There was a problem hiding this comment.
We have to use this twice. I wonder if a small free reverse_erase function would be valuable.
| } | ||
| else | ||
| { | ||
| // The motion model failed. Stop further processing and try again next time. | ||
| break; | ||
| // The motion model failed. Stop further processing of this sensor and try again next time. |
There was a problem hiding this comment.
Just thinking through the cases here. We're iterating in reverse, so if we just happen to have some 'bad' transaction the the motion model didn't like, it will fail to process it at every cycle, which is fine. Eventually, it will be so old that the transaction will expire and be removed. Newer measurements from that same sensor will be processed first in future iterations, so we're not blocking new data from that sensor form being processed. If the measurement was just too new, it'll get picked up in a future iteration.
There was a problem hiding this comment.
Newer measurements from that same sensor will be processed first in future iterations, so we're not blocking new data from that sensor form being processed.
Just want to be clear. We are processing transactions from each sensor, oldest first. Thus, if a transaction is failing to generate motion models, no newer transactions from that sensor will be added to the graph until the problem transaction times out and is removed.
There was a problem hiding this comment.
So I get that we're blacklisting single sensors, but I guess what concerns me is a 'bad' measurement from sensor A prevents all future measurements from it until it gets dropped from the queue. But it's much better than the current setup.
There was a problem hiding this comment.
I'm taking the "it's much better than the current setup" as a win for now. We can discuss a more advanced system for handling failures in the future.
| } | ||
| // Attempt to process each pending transaction | ||
| while (!pending_transactions_.empty()) | ||
| auto sensor_blacklist = std::vector<std::string>(); | ||
| auto transaction_iter = pending_transactions_.rbegin(); |
There was a problem hiding this comment.
Maybe rename to transaction_riter to signal it's a reverse iterator 🤔
Reworked the transaction queue to skip transactions on a per-sensor basis.