Problems with the Heartbeat

[arthurbenemann]

There are a couple of problems with the current Heartbeat implementation. Here is the file: https://github.com/arthurbenemann/droidplanner/blob/master/DroidPlanner/src/com/droidplanner/MAVLink/MavLinkHeartbeat.java

• I think it should use a Executor to improve the performance instead of a handler:

scheduleTaskExecutor = Executors.newScheduledThreadPool(5);
            scheduleTaskExecutor.scheduleWithFixedDelay(new Runnable() {

• 1 Hz isn't too much? Maybe that should be slowed down to 0.5 or 0.2, better yet if it's preference settable (with the otther mavlink stream rates).
• It should be moved to a new place as the GCSHeartbeat, but keeping the mavlink message packing where it is now as an static method.

[Paku-]

Is this not working or you just would like to make it less cpu consuming ?

[arthurbenemann]

It's working, but it is generating a lot of lose objects. You can see that if you enable/ disable this code and what the Garbaje Colector needing to run a lot more.

Also it could use some refactoring, since the com.droidplanner.MAVLink package should only contain static methods to send mavlink messages (kind of an interface between the app and the mavlink library).

Azrin code works without problems, but we can always improve things :)

[Paku-]

Why static ??

[arthurbenemann]

So there is no need for an object allocation, for instance take a look at the following lines: https://github.com/arthurbenemann/droidplanner/blob/master/DroidPlanner/src/com/droidplanner/MAVLink/MavLinkModes.java#L31-L37

[Paku-]

But you mean this part only:

https://github.com/arthurbenemann/droidplanner/blob/master/DroidPlanner/src/com/droidplanner/MAVLink/MavLinkHeartbeat.java

or whole package should be refactored ...

Is the MavLink package this projects code or a kind of library taken from the net ??

[m4gr3d]

@arthurbenemann @Paku- , additionally this code:

public static void sendMavHeartbeat(Drone drone) {
                msg_heartbeat msg = new msg_heartbeat();
                msg.type = MAV_TYPE.MAV_TYPE_GCS;
                msg.autopilot = MAV_AUTOPILOT.MAV_AUTOPILOT_GENERIC;        
                if(drone!=null)
                drone.MavClient.sendMavPacket(msg.pack());
        }

creates a new msg_heartbeat everytime it's called, even though the msg_heartbeat ends up being the same. Given it's called in a pretty fast 'loop' via handler, that's recipe for disasters in java. You never want to do object allocation in a fast loop in java.

The solution would be to create a static msg_heartbeat object once, and reuse the same object everytime you call sendMavHeartbeat . Unless i misread the code, the `msg_heartbeat object(s) doesn't need to be different for each call to sendMavHeartbeat. Feel free to correct me if i'm wrong.

[arthurbenemann]

This part should be keep on the com.droidplanner.MAVLink.MavLinkHeartbeat.java object: https://github.com/arthurbenemann/droidplanner/blob/master/DroidPlanner/src/com/droidplanner/MAVLink/MavLinkHeartbeat.java#L44-L50

The rest should be extracted to a new class, I suggest com.droidplanner.gcs.HeartbeatGCS since this is the heartbeat of the GCS.

Eclipse has some great tools to do it, and put it here as a reminder for me to do it. But if someone what's to make an attempt at it you are free to do it.

[arthurbenemann]

@ne0fhyk The object is the same every time, but first we just need the refactoring to make the code more readable and decoupled. The implementation of the Heartbeat message can be changed later, since 1Hz isn't all that much.

[Paku-]

But I still do not know what the rest is ??? All classes under the com.droidplanner.MAVLink ?? You want all of them to be more static ?? Or GC friendly ??

and why this one part should be kept here ??

I can work on it but have to know what the goal is ....

[m4gr3d]

@arthurbenemann , the reason you see the extra garbage collection is because of the creation of new msg_heartbeat objects in sendMavHeartbeat, and new MAVLinkPacket objects when calling msg.pack() in sendMavHeartbeat. The fact that the MavLinkHeartbeat is not fully 'static' doesn't matter much since it's only created once, in the SuperUI#onCreate() method.

If you refactor the MavLinkHeartbeat class into a 'static' class, which component would be in charge of calling it to dispatch the heartbeats?

Since the message's frequency is only 1Hz, using Executor is not really necessary. It would make more sense to use it if we were handling and managing several threads with much higher demands (http://stackoverflow.com/questions/7462098/handlerthread-vs-executor-when-is-one-more-appropriate-over-the-other)

[Paku-]

Agree :)

[m4gr3d]

@Paku- I think I understand what @arthurbenemann means by refactoring that code. I'll take care of it :)

[Paku-]

ok, but is this true Java have has no object.free method ??

[arthurbenemann]

The idea is to keep https://github.com/arthurbenemann/droidplanner/blob/master/DroidPlanner/src/com/droidplanner/MAVLink/MavLinkHeartbeat.java#L44-L50 where it is, and move all the rest of that class into a new class at com.droidplanner.gcs.HeartbeatGCS. After that change the handler code to an ScheduledExecutor.

@ne0fhyk The idea of using a ScheduledExecutor is because it's used at other places at the code, and IMO it's a cleaner API.

443 is a better implementation (already merged), but we still can benefit from decoupling the GCS heartbeat object, from the MAVlink implementation as described above.

@Paku- There is no need for the object.free() method since the Garbage Collector handles that when there are no more references to an object.

[m4gr3d]

@Paku- , yea java has a garbage collector that runs and clean up unused objects in the background. The best a developer can do is to make sure unused objects don't have remaining references in other used/live objects, so the garbage collector know that they are eligible to collection (http://stackoverflow.com/questions/1582209/java-garbage-collector-when-does-it-collect)

[Paku-]

@arthurbenemann It was a little need here .... @ne0fhyk copy ..

[m4gr3d]

@arthurbenemann I'm decoupling the code and will submit a pull in a few minutes.

Actually, i would argue that using Handler is cleaner since it's part of the android platform. For what the GCSHeartbeat does, ScheduledExecutor is overblown, and too heavy because of all the extra functionality it brings. IMO, ScheduledExecutor would be appropriate if we were to merge all the gcs, and other components' functionality into one class, and have this class handles all these threads using scheduledExecutor.

[arthurbenemann]

The ScheduledExecutor may be too much for this simple task, and sure a Handler can execute that job. My only argument is that the code looks much better with the SheduledExecutor.

As an example take a look at the current implementation using a handler, it takes a lot of user code to make the handler continue executing. Also there is the need of maintaining a tracking variable to see/set if the handler is running or not. https://github.com/arthurbenemann/droidplanner/blob/master/DroidPlanner/src/com/droidplanner/helpers/RcOutput.java#L57-L63

The ScheduledExecutor on the other hand looks cleaner, tightly coded. Just a look over it and you know what is happening, no need to decipher what makes the handler tick. https://github.com/arthurbenemann/droidplanner/blob/master/DroidPlanner/src/com/droidplanner/MAVLink/MavLinkHeartbeat.java#L40-L60

But anyway this is just my opinion, either one will work. We are just discussing whether to mow the lawn with an electric or petrol lawn-mower. And thanks for the discussion it's very instructive.

[m4gr3d]

No problem, and you managed to convince me on using the ScheduledExecutor :) Though, in RcOutput you're calling Executors.newScheduledThreadPool(5), while only one thread is needed, so Executors.newSingleThreadScheduledExecutor() should be better.

[arthurbenemann]

The issue was been closed at #447.

[azrin1972]

I’m working on the mavlinkmsghandling now. will send you the codes for review before pull request

On Dec 17, 2013, at 12:12 AM, Arthur Benemann notifications@github.com wrote:

It's working, but it is generating a lot of lose objects. You can see that if you enable/ disable this code and what the Garbaje Colector needing to run a lot more.

Also it could use some refactoring, since the com.droidplanner.MAVLink package should only contain static methods to send mavlink messages (kind of an interface between the app and the mavlink library).

Azrin code works without problems, but we can always improve things :)

— Reply to this email directly or view it on GitHub.