Revision history [back]

I know this is a late answer but I'm adding it after doing a review of someones code and found they had implemented it blindly because it is "recommended, of course!" by kramer.

Why do you need to move calculations outside the callback? The only case it's really valid is if you are filtering messages. Ie you expect more messages than your module can/wants to process so you need to throw away some of them, in which case the subscriber message queue will fill up so you may not be processing the latest message.

In that case either add more AsyncSpinner threads to process your callback or have a calculation thread running to process your calculations. If you use multiple AsyncSpinners then use mutexes for the data accessed across mutliple threads or make your callback stateless (only depend on the variables passed into the callback) - which is a good idea anyway.

You can also have the callback post a message into a queue (or post a semaphore) which is exactly what you did to get the message into the callback in the first place and have another thread blocking on a queue receive (or a semaphore pend) then process your data. This is the correct implementation of the producer - consumer method.

If you are running in any normal system where you aren't spamming your modules with messages and you want to process all the messages, then handle it in the callback. Handling all the processing in the callback makes the code easier to read and maintain and actually REDUCES the processing load because you are not needlessly passing data around. The data has to be processed somewhere, the more it's passed around the more overhead is added. The above answer by kramer suggests moving the processing to a Timer thread. This is the worst method of all. It means that you are now adding a polling loop into your code. It's just a pain - don't do it.

1. You have an event driven architecture given to you by ros and are using it! Don't use it to then create a polling architecture.

2. Unless you implement some checking, you won't know if you're dropping messages. Ie if 2 messages come in before the timer goes off.

3. You're adding a delay between receiving the message and processing it. If you're passing the message between many module then you might be adding a ton of latency into your system and if you aren't adding delay then you're adding a ton of processing because of the fast polling loop.

4. It's extremely difficult to profile your code and work out how much load you're inducing. If you're not sending any messages your loops are all still running checking for messages. If you're sending around lots of messages you can still only process them as fast as your polling loop so you'll likely be dropping messages and not notice. In event driven systems you can tell you CPU load by, funnily enough, looking at the CPU load and message queue sizes.

5. You're just wasting CPU. If all your code does this, it's just bad news for performance. In an event driven architecture which you would have with the processing inside the callback, the Operating system will run the threads that need to be run - when a message arrives. When polling, the Operating system is constantly switching between threads to run a poll loop that doesn't to do anything because there's no message to receive.

6. There is tight coupling between the rest of the system and the polling frequency. This is the worst bit of all. If you want your code to execute with little latency (as everyone does) then you need a fast polling loop which is not scale able across a large system.

The fact is that the processing has to happen somewhere!! moving it to another block of software is just delaying it and making your code harder to follow. Make your code as simple to read and understand as possible it is more maintainable that way - this is best achieved using the built in methods eg callbacks and add more aynsc spinners to increase parallelism (only if you have a multicore CPU and the rest of the system doesn't need the CPUs, otherwise you are again, slowing things down).

Having said that there is always a use case for something. Just keep things simple and use the stupid methods (eg moving your processing to a timer loop) sparingly.