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

Posted by murata,katsutoshi on July 15, 2016 at 10:33pm

dronecodeのコーディングルールを拝見いたしました。

あまりにもざっくりしていることもあり、保守性や品質まではいき渡っていなさそうと感じています。

そこで、日本には、組み込み系ソフトについて、コーディングルールというのを事細かく定義してるものがあります。

例えば、

・MISRAとか

・IPAの「組込みソフトウェア開発向けコーディング作法ガイド[C＋＋言語版]」や

・凡ミスによる障害の回避ルール

これらのルールをベースにArduCopterのコードの品質を上げていきたい。

また、その過程で、不安なコードがあれば、不安を軽減していきたい。

その情報や修正案などのアップしたいと思い、このディスカッションを立ち上げました。

何卒、よろしくおねがいいたします。

You need to be a member of diydrones to add comments!

Join diydrones

Tags: arducopter, coding, ipa, misra, rule
Email me when people reply –
Follow

Replies

  • Goro Senzai August 4, 2016 at 11:48pm

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


    murata,katsutoshi said:

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

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

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

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

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

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

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

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

    3702278766?profile=original

  • Goro Senzai August 4, 2016 at 11:43pm

    Pixhawkは時計用のバックアップバッテリーが無いので、これは仕方ないかなと思います。

    murata,katsutoshi said:

    Mission Planner で表示されるログ時間が時刻更新されるまで 2000/1/1 になっているようです。

    この時刻設定について改善したいです。

    3702277076?profile=original

  • murata,katsutoshi August 4, 2016 at 11:39pm

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

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

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

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

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

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

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

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

    3702278766?profile=original

  • murata,katsutoshi August 4, 2016 at 12:34am

    AC_PolyFence_loader.cpp の load_point_from_eepromメソッド、及び、save_point_to_eepromメソッドで、フェンスポイントYの取得／設定で、マジックナンバー「４」を使用しています。

    これは、フェンスポイントXの次の位置、から取得、へ設定、するためにしている処理と思われます。

    そこで、マジックナンバー「４」を使用せずに、sizeof(uint32_t)にすることで、保守性をあげたいと思いPR致しました。

  • murata,katsutoshi August 2, 2016 at 1:23am

    Mission Planner で表示されるログ時間が時刻更新されるまで 2000/1/1 になっているようです。

    この時刻設定について改善したいです。

    3702277076?profile=original

  • murata,katsutoshi July 29, 2016 at 3:38am

    この PR ソースに組み込まれました。(^^)

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

    ＝＝＝

    このソースについてどのように値を確認したかをご紹介いたします。

    SITLで値を確認するため、USB型プロポを使用しています。

    シミュレートしながら値の確認ができます。

    下記がその出力するための処理です。

    １秒にしているのは、出力が遅いため、間引きしています。

        static uint32_t before_time = 0u;

        if((millis() - before_time) > 1000u) {
            hal.console->printf("Drift target_roll=[%f]\n", target_roll);  // TODO Debug
            before_time = millis();
        }
    画像は、その値です。

    3702274973?profile=original
    murata,katsutoshi said:

    こちらについて、PRいたしました。

    branch/masterにコミットしちゃったので、ないものにしたかったんですが。。。
    やり方の問題で、さらに悪化してしまいました。
    今の状態からfork時の状態に戻せる知識がなく、レポジトリを削除して再度forkしました。

    コメントやPRの仕方をググって真似て、再度、こちらをPRいたしました。

    度数を100倍しているのでさらに細かい値が必要か?ということになりそうです。



    murata,katsutoshi said:

    control_drift.cpp の drift_run() メソッドについて

    現状：

    target_roll変数は、float変数にもかかわらず、constrain_int16を使用しています。

        // Roll velocity is feed into roll acceleration to minimize slip
        target_roll = roll_vel_error * -DRIFT_SPEEDGAIN;
        target_roll = constrain_int16(target_roll, -4500, 4500);

    他については、constrain_floatを使用しております。

    この変数、あるリビジョンでint16からfloatに変更になりました。

    しかし、この処理については変更していませんでした。

    これは、変更漏れと思われます。

    改善案：

        target_roll = constrain_float(target_roll, -4500.0f, 4500.0f);

  • murata,katsutoshi July 28, 2016 at 4:22pm
    こちらについて、PRいたしました。

    branch/masterにコミットしちゃったので、ないものにしたかったんですが。。。
    やり方の問題で、さらに悪化してしまいました。
    今の状態からfork時の状態に戻せる知識がなく、レポジトリを削除して再度forkしました。

    コメントやPRの仕方をググって真似て、再度、こちらをPRいたしました。

    度数を100倍しているのでさらに細かい値が必要か?ということになりそうです。



    murata,katsutoshi said:

    control_drift.cpp の drift_run() メソッドについて

    現状：

    target_roll変数は、float変数にもかかわらず、constrain_int16を使用しています。

        // Roll velocity is feed into roll acceleration to minimize slip
        target_roll = roll_vel_error * -DRIFT_SPEEDGAIN;
        target_roll = constrain_int16(target_roll, -4500, 4500);

    他については、constrain_floatを使用しております。

    この変数、あるリビジョンでint16からfloatに変更になりました。

    しかし、この処理については変更していませんでした。

    これは、変更漏れと思われます。

    改善案：

        target_roll = constrain_float(target_roll, -4500.0f, 4500.0f);

  • murata,katsutoshi July 27, 2016 at 4:29am
    コメントありがとうございます。。

    ご提示いただきました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しています。
    3702269050?profile=original

  • Goro Senzai July 26, 2016 at 7:10pm

    これに関しては、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しています。
    3702269050?profile=original

    Contributing Code — Dev documentation
  • murata,katsutoshi July 26, 2016 at 3:34pm

    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しています。
    3702269050?profile=original

This reply was deleted.