Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Various changes #34

Merged
merged 17 commits into from Feb 15, 2012

Conversation

Projects
None yet
3 participants

Here's a bunch of improvements that I've made. Most of these are related to Windows compilation and new autopilot integration.

Bryant Mairs added some commits Jan 17, 2012

Bryant Mairs Fixed unused variable warning in WaypointList.cc 55ed021
Bryant Mairs Added tooltip to all the appropriate labels on the toolbar.
Also reorganized the code to be more readable.
6dbf6da
Bryant Mairs Merge remote-tracking branch 'remotes/upstream/master' 5a0a929
Bryant Mairs Added support for reading uint8 values from an autopilot via QGC. Sti…
…ll need to add support for writing them.
6821ff4
Bryant Mairs Removed some sources for warnings.
Also removed redundant code in QGCMapWidget.cc.
fcdf550
Bryant Mairs Fixed problems copying support files during Windows compilation. 75792cc
Bryant Mairs Compilation on Windows VS2010 now properly copies necessary support l…
…ibraries to the /debug and /release directories based on which is currently building. These copies are also only done if they are necessary.

I removed copying of the plugins folder from Qt to the executable directory as I don't believe it's necessary anymore.
216b023
Bryant Mairs Merge branch 'master' of https://github.com/mavlink/qgroundcontrol c2a4f4d
Bryant Mairs Merge branch 'master' of https://github.com/mavlink/qgroundcontrol 2b43975
Bryant Mairs Refactored part of MAVLinkProtocol::receiveBytes() as I was trying to…
… debug within it. Nothing major, though I did remove an unnecessary loop.
7dfac40
Bryant Mairs Added a workaround to the mission management code where it would only…
… accept messages addressed to the MAV_ID_MISSIONPLANNER component. This was causing problems with the send-loss value being calculated, so I decided to add this until a proper solution can be developed.
260e9ca
Bryant Mairs Expanded documentation for UASInfoWidget::update*Loss.
Changed the drop rate from a SYS_STATUS message to follow the MAVLink specs along with some extra range checking.
b03070d
Bryant Mairs Merge branch 'master' of https://github.com/mavlink/qgroundcontrol
Conflicts:
	qgroundcontrol.pri
	src/uas/UAS.cc
e0635ae
Bryant Mairs Removed redundant boolean comparison. e58b34c
Bryant Mairs Fixed unused variable warning during compilation. 7ce3400
Bryant Mairs Re-add copying of /models directory that was accidentally removed dur…
…ing last merge.
c3ebf07
Bryant Mairs Fixed borked Windows compilation introduced with mavlink/qgroundcontr…
…ol:864e6ae8.
19805a9

@ghost ghost pushed a commit that referenced this pull request Feb 15, 2012

@LorenzMeier LorenzMeier Merge pull request #34 from Susurrus/master
Various changes
27932f5

@ghost ghost merged commit 27932f5 into mavlink:master Feb 15, 2012

@ghost

ghost commented Feb 15, 2012

Highly appreciated, all merged in.

The hardcoded component ID in mission / waypoint handling has been identified and is currently work is done to resolve it properly. Your immediate fix is the right thing in the meantime though.

https://github.com/Trof/qgroundcontrol-1/tree/no-osg

On Feb 15, 2012, at 8:39 PM, Susurrus wrote:

Here's a bunch of improvements that I've made. Most of these are related to Windows compilation and new autopilot integration.

You can merge this Pull Request by running:

git pull https://github.com/Susurrus/qgroundcontrol master

Or you can view, comment on it, or merge it online at:

#34

-- Commit Summary --

  • Fixed unused variable warning in WaypointList.cc
  • Added tooltip to all the appropriate labels on the toolbar.
  • Merge remote-tracking branch 'remotes/upstream/master'
  • Added support for reading uint8 values from an autopilot via QGC. Still need to add support for writing them.
  • Removed some sources for warnings.
  • Fixed problems copying support files during Windows compilation.
  • Compilation on Windows VS2010 now properly copies necessary support libraries to the /debug and /release directories based on which is currently building. These copies are also only done if they are necessary.
  • Merge branch 'master' of https://github.com/mavlink/qgroundcontrol
  • Merge branch 'master' of https://github.com/mavlink/qgroundcontrol
  • Refactored part of MAVLinkProtocol::receiveBytes() as I was trying to debug within it. Nothing major, though I did remove an unnecessary loop.
  • Added a workaround to the mission management code where it would only accept messages addressed to the MAV_ID_MISSIONPLANNER component. This was causing problems with the send-loss value being calculated, so I decided to add this until a proper solution can be developed.
  • Expanded documentation for UASInfoWidget::update*Loss.
  • Merge branch 'master' of https://github.com/mavlink/qgroundcontrol
  • Removed redundant boolean comparison.
  • Fixed unused variable warning during compilation.
  • Re-add copying of /models directory that was accidentally removed during last merge.
  • Fixed borked Windows compilation introduced with mavlink/qgroundcontrol:864e6ae8.

-- File Changes --

M qgroundcontrol.pri (76)
M qgroundcontrol.pro (36)
M qgroundcontrol.qrc (4)
M src/comm/MAVLinkProtocol.cc (58)
M src/comm/MAVLinkProtocol.h (2)
M src/uas/UAS.cc (14)
M src/uas/UASWaypointManager.cc (8)
M src/ui/HSIDisplay.cc (3)
M src/ui/QGCToolBar.cc (44)
M src/ui/WaypointList.cc (4)
M src/ui/map/QGCMapWidget.cc (31)
M src/ui/mavlink/QGCMAVLinkMessageSender.cc (3)
M src/ui/uas/UASInfoWidget.h (14)

-- Patch Links --

https://github.com/mavlink/qgroundcontrol/pull/34.patch
https://github.com/mavlink/qgroundcontrol/pull/34.diff


Reply to this email directly or view it on GitHub:
#34


Lorenz Meier
PhD Student
Computer Vision and Geometry Lab
ETH Zurich /
Swiss Federal Institute of Technology
http://www.inf.ethz.ch/personal/lomeier/

@TobiasSimon TobiasSimon commented on the diff Feb 18, 2012

src/comm/MAVLinkProtocol.cc
- // while (lastCompIndex->value(message.compid, 0)+1 )
- // }
- //if ()
+ // NOTE: Using uint8_t here auto-wraps the number around to 0.
+ expectedIndex = lastIndex[message.sysid][message.compid] + 1;
+ }
+
+ // Make some noise if a message was skipped
+ //qDebug() << "SYSID" << message.sysid << "COMPID" << message.compid << "MSGID" << message.msgid << "EXPECTED INDEX:" << expectedIndex << "SEQ" << message.seq;
+ if (message.seq != expectedIndex)
+ {
+ // Determine how many messages were skipped accounting for 0-wraparound
+ int16_t lostMessages = message.seq - expectedIndex;
+ if (lostMessages < 0)
+ {
+ lostMessages += 256;
@TobiasSimon

TobiasSimon Feb 18, 2012

Contributor

Hi!

This piece of code causes huge problems with out-of-order packets, which may occur when using UDP via some intermediate network nodes. We are facing serious loss rate peaks (up to 30%) each time a message is out-of-order, like in the sequence [2,1]. In this example we expect after 3 to be the next seq number, but we get 1, so lostMessages = -2. The if yields lostMessages = 254.. which is quite a lot!

Accoding to http://www.wireshark.org/lists/wireshark-users/201112/msg00030.html, wireshark calculates packet loss via (SEQ_N - SEQ_N-1 - 1) in RTP, which is exactly the same as lostMessages = message.seq - expectedIndex in your code. The difference is that they allow this value to be negative.

So maybe one solution would be to remove the "if" or just set lostMessages to 0 instead of 256.

Regards,
Tobias

@ghost

ghost Feb 18, 2012

Can you prototype this solution?

I'm aware of several pull requests you have issued, please merge in the code from master however first. I'm happy to mere it in, up until now it creates however merge conflicts for which I need substantially more time to resolve and then validate. If you handle this, I can merge faster.

--- Ursprüngl. Mitteilung ---
Von:Tobias Simon
Gesendet: 18-02-2012, 09:27
An: Meier Lorenz
Betreff:Re: [qgroundcontrol] Various changes (#34)

  •            //                while (lastCompIndex->value(message.compid, 0)+1 )
    
  •            //            }
    
  •            //if ()
    
  •                                 // NOTE: Using uint8_t here auto-wraps the number around to 0.
    
  •                expectedIndex = lastIndex[message.sysid][message.compid] + 1;
    
  •                         }
    
  •                         // Make some noise if a message was skipped
    
  •            //qDebug() << "SYSID" << message.sysid << "COMPID" << message.compid << "MSGID" << message.msgid << "EXPECTED INDEX:" << expectedIndex << "SEQ" << message.seq;
    
  •                         if (message.seq != expectedIndex)
    
  •                         {
    
  •                                 // Determine how many messages were skipped accounting for 0-wraparound
    
  •                                 int16_t lostMessages = message.seq - expectedIndex;
    
  •                                 if (lostMessages < 0)
    
  •                                 {
    
  •                                         lostMessages += 256;
    

Hi!

This piece of code causes huge problems with out-of-order packets, which may occur when using UDP via some intermediate network nodes. We are facing serious loss rate peaks (up to 30%) each time a message is out-of-order, like in the sequence [2,1]. In this example we expect after 3 to be the next seq number, but we get 1, so lostMessages = -2. The if yields lostMessages = 254.. which is quite a lot!

Accoding to http://www.wireshark.org/lists/wireshark-users/201112/msg00030.html, wireshark calculates packet loss via (SEQ_N - SEQ_N-1 - 1), which is exactly the same as lostMessages = message.seq - expectedIndex in your code. The difference is that they allow this value to be negative.

So maybe one solution would be to remove the "if" or just set lostMessages to 0 instead of 256.

Regards,
Tobias


Reply to this email directly or view it on GitHub:
https://github.com/mavlink/qgroundcontrol/pull/34/files#r464652

@TobiasSimon

TobiasSimon Feb 18, 2012

Contributor

OK, thanks for the info. I'll close the broken pull requests and send a cumulative one with your master merged.

@TobiasSimon

TobiasSimon Feb 18, 2012

Contributor

I have tested the two still open pull requests:
#41 and #43 are based on aaf236a, which is the current master.
What you mean is pull request 37, I think. I figured out that it was based on an older commit and closed it myself.
I think that they remaining two should apply without merge conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment