ArduPilotは「組み込みソフト」、だったら保守性や品質をあげていきたい。

Goro Senzai August 13, 2016 at 5:27am

ありがとうございます。

確かにLOG_DISARMEDの独立パラメーター化はmasterにしか反映されていないので、Copterの現行バージョンを使う場合は、ご指摘のようにビットマスクを使うのがが正解です。

Tomi Mizya said:

お望みの機能はコレですかね？

Goro Senzai August 13, 2016 at 5:13am

おめでとうございます。コミットのコメントもLucasさんが簡潔に書き換えてくれたようですね。

https://github.com/ArduPilot/ardupilot/commit/4eee3b1317b954c7c1df1...

murata,katsutoshi said:

Lucas De Marchiさんから、最終コメントをいただきました。

せんざいさんに、プッシュしていただいた結果です。

ありがとうございました。

The fix is correct since we are using an unsigned value an in the case fast_loop method takes that long this would underflow, allowing an arbitrary high time for this loop iteration.

If your fast_loop() is taking that long you are screwed, but we at least can fix the underflow.

：

murata,katsutoshi said:

コメントありがとうございます。

修正して、PRいたしました。

修正内容：

runメソッドの引用部内で選択するようにいたしました。

murata,katsutoshi said:

コメントありがとうございます。。

ご提示いただきましたurlを参照して対処したいと思います。

今後もよろしくお願いいたします。

Goro Senzai said:
これに関しては、Pull Requestのコメントにも書かれていますが、複数箇所のコードを同時に変更しているため、Unrelatedと指摘されています。

Gitの作法に関しては、PRでもRandyさんやmagicrubさんが丁寧に説明してくれていますが、他のオープンソースコミュニティーと同じく、ArduPilotにも作法がありまして、Pull Requestに関しては：

- masterで開発せず、別の（develop)等で開発を行い、最新のmasterにrebaseしてからPRを投げる

- 同じモジュール・ライブラリー内の最小限の変更のみに留める（例えば、Copter, Plane, Roverで同じ変更があったとしても、それぞれ別のPRとして投げる）

- PRを投げる前に、必ず自分の環境でテストする

- 改行はLFを使う

- タブを使わずにスペース4つ

など、いろいろあります。

詳しくはこちらを読んでみてください。

http://ardupilot.org/dev/docs/contributing.html

特にこのページ：

http://ardupilot.org/dev/docs/submitting-patches-back-to-master.html

あと、PRのリストを眺めて、コメントを読んだり、複数モジュールの同一変更がどう分割されてsubmitされているのか等をみるのも参考になると思います。

では、めげずに頑張ってください！

murata,katsutoshi said:

Tom Pittengerさんから、「This PR contains unrelated changes」とコメントがありました。

この改善案は、fast_loop が2500usを超えた場合を想定しています。

そういうことが「無い」と断言できれば、不要な処理となります。

「無い」ってことですね。

不安が一つなくなりました。

murata,katsutoshi said:

こちらも、Pull Requestいたしました。

さて、どのようなコメントをいただけますか、楽しみでもあり、。。。小心者なので。。。

murata,katsutoshi said:

・ArduCopter.cpp の loop() メソッド

fast_loop()メソッドが2500usを超えると以下の「uint32_t time_available」の値が不正になります。

さらに、その直後のscheduler.run()の引数の型がなんとuint16_tです。

    // run all the tasks that are due to run. Note that we only
    // have to call this once per loop, as the tasks are scheduled
    // in multiples of the main loop tick. So if they don't run on
    // the first call to the scheduler they won't run on a later
    // call until scheduler.tick() is called again
    uint32_t time_available = (timer + MAIN_LOOP_MICROS) - micros();　
    scheduler.run(time_available);

改善案：

    uint32_t now = micros();　

    uint32_t time_available = ((timer + MAIN_LOOP_MICROS) < now) ? 0 : (timer + MAIN_LOOP_MICROS) - now ;　

画像は、もし、fast_loop()メソッドが2500usを超える場合を想定してdelay()メソッドにて2msしています。

Copter: fix underflow in scheduler · ArduPilot/ardupilot@4eee3b1

If fast_loop method executed time is over MAIN_LOOP_MICROS, scheduler.run method set value is 0.

murata,katsutoshi August 11, 2016 at 1:21pm

setup.cpp の esc_calib メソッドで、入力文字コードですでに「'c'」をチェックしており、0x63でチェックする意は？

不要なチェックと思うんだけど。

        if (c == 'c') {
            break;

       } else if (c == 0x03 || c == 0x63 || c == 'q') {

murata,katsutoshi August 10, 2016 at 4:19pm

Lucas De Marchiさんから、最終コメントをいただきました。

せんざいさんに、プッシュしていただいた結果です。

ありがとうございました。

The fix is correct since we are using an unsigned value an in the case fast_loop method takes that long this would underflow, allowing an arbitrary high time for this loop iteration.

If your fast_loop() is taking that long you are screwed, but we at least can fix the underflow.

：

murata,katsutoshi said:

コメントありがとうございます。

修正して、PRいたしました。

修正内容：

runメソッドの引用部内で選択するようにいたしました。

murata,katsutoshi said:

コメントありがとうございます。。

ご提示いただきましたurlを参照して対処したいと思います。

今後もよろしくお願いいたします。

Goro Senzai said:
これに関しては、Pull Requestのコメントにも書かれていますが、複数箇所のコードを同時に変更しているため、Unrelatedと指摘されています。

Gitの作法に関しては、PRでもRandyさんやmagicrubさんが丁寧に説明してくれていますが、他のオープンソースコミュニティーと同じく、ArduPilotにも作法がありまして、Pull Requestに関しては：

- masterで開発せず、別の（develop)等で開発を行い、最新のmasterにrebaseしてからPRを投げる

- 同じモジュール・ライブラリー内の最小限の変更のみに留める（例えば、Copter, Plane, Roverで同じ変更があったとしても、それぞれ別のPRとして投げる）

- PRを投げる前に、必ず自分の環境でテストする

- 改行はLFを使う

- タブを使わずにスペース4つ

など、いろいろあります。

詳しくはこちらを読んでみてください。

http://ardupilot.org/dev/docs/contributing.html

特にこのページ：

http://ardupilot.org/dev/docs/submitting-patches-back-to-master.html

あと、PRのリストを眺めて、コメントを読んだり、複数モジュールの同一変更がどう分割されてsubmitされているのか等をみるのも参考になると思います。

では、めげずに頑張ってください！

murata,katsutoshi said:

Tom Pittengerさんから、「This PR contains unrelated changes」とコメントがありました。

この改善案は、fast_loop が2500usを超えた場合を想定しています。

そういうことが「無い」と断言できれば、不要な処理となります。

「無い」ってことですね。

不安が一つなくなりました。

murata,katsutoshi said:

こちらも、Pull Requestいたしました。

さて、どのようなコメントをいただけますか、楽しみでもあり、。。。小心者なので。。。

murata,katsutoshi said:

・ArduCopter.cpp の loop() メソッド

fast_loop()メソッドが2500usを超えると以下の「uint32_t time_available」の値が不正になります。

さらに、その直後のscheduler.run()の引数の型がなんとuint16_tです。

    // run all the tasks that are due to run. Note that we only
    // have to call this once per loop, as the tasks are scheduled
    // in multiples of the main loop tick. So if they don't run on
    // the first call to the scheduler they won't run on a later
    // call until scheduler.tick() is called again
    uint32_t time_available = (timer + MAIN_LOOP_MICROS) - micros();　
    scheduler.run(time_available);

改善案：

    uint32_t now = micros();　

    uint32_t time_available = ((timer + MAIN_LOOP_MICROS) < now) ? 0 : (timer + MAIN_LOOP_MICROS) - now ;　

画像は、もし、fast_loop()メソッドが2500usを超える場合を想定してdelay()メソッドにて2msしています。

murata,katsutoshi August 10, 2016 at 4:02pm

その他：ログ解析で原因が特定しやすくすることも保守性をあげることにつながるので検討してみます。

http://diydrones.com/group/japan-arducopter-group/forum/topics/gps-3

Log File C:\Users\muratakatsutoshi\AppData\Local\Temp\tmp448B.tmp.log
Size (kb) 2196.357421875
No of lines 26936
Duration 0:03:12
Vehicletype ArduCopter
Firmware Version V3.3.2
Firmware Hash 7f16e4d6
Hardware Type
Free Mem 0
Skipped Lines 0

Test: Autotune = NA -
Test: Balance/Twist = NA -
Test: Brownout = GOOD -
Test: Compass = WARN - Moderate change in mag_field (30.72%)

Test: Dupe Log Data = GOOD -
Test: Empty = GOOD -
Test: Event/Failsafe = FAIL - ERR found: CRASH
Test: GPS = GOOD -
Test: IMU Mismatch = GOOD - (Mismatch: 0.40, WARN: 0.75, FAIL: 1.50)
Test: Parameters = GOOD -
Test: PM = NA -
Test: Pitch/Roll = NA -
Test: Thrust = NA -
Test: VCC = GOOD -

murata,katsutoshi August 6, 2016 at 8:50am

Tom Pittengerさんから、最終コメントをいただきました。

In theory you could run a quadplane at 300 hz until it is in fixed wing mode then back to 50 during forward flight. Would be sketchy but in general I think we should strive to have all params work instantly and not require a reboot. Closing.

murata,katsutoshi said:

ArduPlane.cpp の loop メソッドの loop_us 変数が auto 変数ということで、ループするたびにメソッド参照しています。

この値はアクウティブに更新されることが無いと思い、static 変数にしてはと、PRいたしました。

uint32_t loop_us = 1000000UL / scheduler.get_loop_rate_hz();

murata,katsutoshi August 5, 2016 at 7:47pm

ArduPlane.cpp の loop メソッドの loop_us 変数が auto 変数ということで、ループするたびにメソッド参照しています。

この値はアクウティブに更新されることが無いと思い、static 変数にしてはと、PRいたしました。

uint32_t loop_us = 1000000UL / scheduler.get_loop_rate_hz();

murata,katsutoshi August 5, 2016 at 5:57pm

コメントありがとうございます。

修正して、PRいたしました。

修正内容：

runメソッドの引用部内で選択するようにいたしました。

murata,katsutoshi said:

コメントありがとうございます。。

ご提示いただきましたurlを参照して対処したいと思います。

今後もよろしくお願いいたします。

Goro Senzai said:
これに関しては、Pull Requestのコメントにも書かれていますが、複数箇所のコードを同時に変更しているため、Unrelatedと指摘されています。

Gitの作法に関しては、PRでもRandyさんやmagicrubさんが丁寧に説明してくれていますが、他のオープンソースコミュニティーと同じく、ArduPilotにも作法がありまして、Pull Requestに関しては：

- masterで開発せず、別の（develop)等で開発を行い、最新のmasterにrebaseしてからPRを投げる

- 同じモジュール・ライブラリー内の最小限の変更のみに留める（例えば、Copter, Plane, Roverで同じ変更があったとしても、それぞれ別のPRとして投げる）

- PRを投げる前に、必ず自分の環境でテストする

- 改行はLFを使う

- タブを使わずにスペース4つ

など、いろいろあります。

詳しくはこちらを読んでみてください。

http://ardupilot.org/dev/docs/contributing.html

特にこのページ：

http://ardupilot.org/dev/docs/submitting-patches-back-to-master.html

あと、PRのリストを眺めて、コメントを読んだり、複数モジュールの同一変更がどう分割されてsubmitされているのか等をみるのも参考になると思います。

では、めげずに頑張ってください！

murata,katsutoshi said:

Tom Pittengerさんから、「This PR contains unrelated changes」とコメントがありました。

この改善案は、fast_loop が2500usを超えた場合を想定しています。

そういうことが「無い」と断言できれば、不要な処理となります。

「無い」ってことですね。

不安が一つなくなりました。

murata,katsutoshi said:

こちらも、Pull Requestいたしました。

さて、どのようなコメントをいただけますか、楽しみでもあり、。。。小心者なので。。。

murata,katsutoshi said:

・ArduCopter.cpp の loop() メソッド

fast_loop()メソッドが2500usを超えると以下の「uint32_t time_available」の値が不正になります。

さらに、その直後のscheduler.run()の引数の型がなんとuint16_tです。

    // run all the tasks that are due to run. Note that we only
    // have to call this once per loop, as the tasks are scheduled
    // in multiples of the main loop tick. So if they don't run on
    // the first call to the scheduler they won't run on a later
    // call until scheduler.tick() is called again
    uint32_t time_available = (timer + MAIN_LOOP_MICROS) - micros();　
    scheduler.run(time_available);

改善案：

    uint32_t now = micros();　

    uint32_t time_available = ((timer + MAIN_LOOP_MICROS) < now) ? 0 : (timer + MAIN_LOOP_MICROS) - now ;　

画像は、もし、fast_loop()メソッドが2500usを超える場合を想定してdelay()メソッドにて2msしています。

Tomi Mizya August 5, 2016 at 7:02am

お望みの機能はコレですかね？

murata,katsutoshi August 5, 2016 at 6:38am

コメントありがとうございます。

AC 3.3.3、3.4.0-rc1 には組み込まれていないようで、残念です。

APは3.6.0で組み込んだようです。

Goro Senzai said:

LOG_DISARMED=1 でARMする前からログが記録されると思います。

murata,katsutoshi said:

室内で、電源を入れただけの状態「DISARMED」で、30分以上、放置していたらmission plannerの画面に「Bad AHRS」が表示されました。

再度、実施したところ、同じ30分ごろに、同メッセージが表示しました。

この現象は、すでに、DIYDronesで上がっていました。

AC3.3.3ではまだ解消していないような感じです。

ログ解析してみようとしたところ、ログファイルが作成されていませんでした。

電源を入れただけだとログファイルを作成するようになっていないのか、まだ、ソースの確認していませんが。。。

初期動作異常を解析するためにもログは必要と思う。

ログファイル作成のタイミングを調査します。

Japan ArduCopter Group

ArduPilotは「組み込みソフト」、だったら保守性や品質をあげていきたい。

Replies