search

ut-issl / s2e-core

Spacecraft Simulation Environment Core codes
MIT License
46 stars 18 forks source link

Make C2A command sender optional #619

Closed sksat closed 6 months ago

sksat commented 6 months ago

Related Issue / PR

Description

C2A command sender feature was implemented by #485. However, this feature is still experimental & optional. To make this explicit, this patch split C2A command sender feature with CMake option and turned off by default.

Impact

200km commented 6 months ago

コマンドセンダーは既にさまざまな設定でdisableできると思います。

その二つがあれば、S2Eの他のコンポーネントと同様に「そのコンポを動かすかはユーザー側で決めることができる」という状態を保てるので、本改修は必要ないように思います。

もしくは、全てのコンポーネントについて「CMakeでON/OFFするようにする」というような方針にしたいということなのかもしれませんが、それは議論が必要になるかなと思います。

sksat commented 6 months ago

いえ，この改修は C2A とのインターフェースを明示的に定義するために必要なものです． 具体的には，SILS-S2E（S2E と C2A を組み合わせた SILS）を行いたいが，C2A に対するテレコマ疎通を別途実装しているようなユースケースにおいて，この機能はリンクしたくないです．

また，この機能は実装時にも experimental な機能であるという扱いをする，ということで合意しているはずで，それを明示したいという意図もあります．

200km commented 6 months ago

「この機能をリンクしたくない」というのはcommand_senderが使われていなくても、クラスや関数として存在しているのが困るということでしょうか？それは「別途実装している機能」となにかしらの名前が被っていてリンクエラーなどがでたりするので、困るということなのでしょうか？具体的な状況を教えてもらえると嬉しいです。

experimentalな機能であるという合意は、「使いたくないユーザーはs2e-userでこのコンポを搭載コンポとして定義しなくて良い」という部分で担保されていると思っています。（実験的なので、全ユーザーが使う必要はない）

sksat commented 6 months ago

（別途実装していて云々，はやや別の話なので，少なくとも初めに挙げる例としては悪かったです．すみません．）

experimental な機能である，というのは，S2E user にとって，というだけでなく，C2A にとって，という部分も含んでいます．少なくとも c2a-core メンテナとしてのお願いとしてはこちらの方がメインでした．これは，この機能・実装をサポートし続けるために C2A が特別の対応をする必要は無いものとしたい，という意図になります．

そのため，SILS-S2E のビルドが（この機能のためだけに）通らなくなるがために C2A（概ね c2a-core）の構造を変えられない，ということは（少なくともまだ）許容できません．なのでこの機能・実装が使えなくなるような変更が（一時的にでも）発生する可能性は非常に多くあります．

例えば，なんらかの機能的な変更でなくとも，src_core/TlmCmd/common_cmd_packet_util.h から include される C2A user のヘッダで C++ ではコンパイルが通らないコードが発生してしまうと，SILS-S2E 全体のビルドが通らなくなり，使えなくなってしまいます（C++ は C言語の上位互換ではなく，非互換は存在します）．

sksat commented 6 months ago

また，c2a-core 側でもこの機能をビルド対象から外したいです．この機能をわざわざ動かなくしたいという意図があるわけではないので，当然できるだけこの機能が新しいバージョンの C2A でも動き続けられるように，S2E へのサポートはしていくつもり（ex: #610 ）ですが，これはあくまで S2E に対する（人間的な）サポートであって，C2A（c2a-core）からのサポート（絶対に維持しなければならない技術的要件）ではないです．

sksat commented 6 months ago

最も分かりやすいところでいうと，今まさに c2a-core の更新で困っています．この機能が明示的に OFF にできれば，

  1. TlmCmd 周りのコードに（command sender から使っているインターフェースの）変更が入る
  2. 一時的に S2E の C2A command sender は動かなくなりますよ，というお知らせを出したりビルドCIでオプションを変えたりする
  3. c2a-core をリリースする
  4. s2e-core に新バージョンの c2a-core 対応のパッチを出す（これは実運用上は動かなくなる期間を短くするために2と同時にやることを努力はする）
  5. s2e-core をリリースする（新しい c2a-core に対応している）

というようなフローを踏めるようになります． しかし，現状では一時的に SILS-S2E 全体のサポートを落とす必要があります．

200km commented 6 months ago

C2A側でS2EのC2A関連コードとの互換が保てない修正が入った時に、C2A側のCIが回らなくなるのが困るということだと理解しました。 この対策としていままでは

というような感じだったと思います。

obc_with_c2aはexperimentalではなく、正式な機能なので必要になったら上のような流れをとるが、今回はcommand_senderが相手でexperimentalなので上の手順を踏むのでなくbuild対象から外して面倒ごとを減らしたいという理解で良いでしょうか？

200km commented 6 months ago

追加で質問ですがこれはC2A側としては修正を急いでいて、修正したらv7.3.0などをリリースしてC2A側でもCIでそれを使いたいかんじでしょうか？

610 もどうようでしょうか？こちらは、私は実はレビューしてApproveおそうとしたのですが、descriptionに sksatとの議論の後isslにレビューと順番の指定があったので、やめました。

sksat commented 6 months ago

CI に限らずですが，そうです > C2A側でS2EのC2A関連コードとの互換が保てない修正が入った時に、C2A側のCIが回らなくなるのが困る

はい．obc_with_c2a で参照しているヘッダ・関数群については，C2A からシミュレータ（S2E および c2a-sils-runtime）へのインターフェースとして明示的にサポートを行うつもりです． > obc_with_c2a は experimental ではなく～

sksat commented 6 months ago

C2A 側としては，https://github.com/arkedge/c2a-core/pull/310 の変更を次のバージョンで入れたいが，この変更のために（一時的に）SILS-S2E ができないリリースを行うことを許容するべきなのか？という判断を迫られている，というのが現状です．なので，このパッチがマージできると SILS-S2E が可能なまま（一時的に，特定のバージョン以前の S2E と特定のバージョン以降の C2A でのみ C2A command sender feature だけが動かなくなるタイミングが発生するものの）リリースが行えるようになります．

sksat commented 6 months ago

610 に関しては，上記の互換性・リリースに関する問題が不透明であるのと，C2A 側の変更もまだ確定しているわけではないので止めている，という状態です

200km commented 6 months ago

obc_with_c2aとcommand_senderがexperimentalかどうかという点で違いがあり、扱われ方が違うという点は理解しました。その場合、command_senderやそれに近い今後実装される可能性のあるc2aに依存するものはどのような手続きを経れば、experimentalでないobc_with_c2aと同等のものになりますか？その手続き方法など教えていただけると嬉しいです。

このパッチがマージできる

マージ、およびminor updateでリリースして良いとは思っていますので、レビューを進めたいと思います。

200km commented 6 months ago

あ、すみません。すでにdevにMajor update要素が入っているので、リリースはできませんね。
C2A側で使っているuser部がGNSSとかを使っているなら、このPRマージ後のdevを使おうとするとエラーなど出る可能性があります。

sksat commented 6 months ago

これはこのパッチを今マージしても，（このパッチ自体は minor update だが）それが使えるようになるのは直近の major update 後になる，という意味ですか？であれば OK です > リリースはできません

これはどういうことでしょう？GNSS 関連のものをこの機能を使ってテストしている C2A user などがあるとこのパッチのマージ後に困ることがある，ということですか？ > C2A側で使っているuser部がGNSSとかを使っているなら、このPRマージ後のdevを使おうとするとエラーなど出る可能性

200km commented 6 months ago

直近の major update 後になる，という意味ですか？

そうなります。

このパッチのマージ後に困ることがある

パッチがmainではなく、developに当てられているので、GNSSの修正が含まれます。 major updateなので、当然ながらIFに修正が入っており、そのIFをuser側が使っていないことを保証できないので、buildエラーは出る可能性もゼロではないです。特殊な使い方をしていないユーザーなら大丈夫な可能性もあります。 パッチがmainに当てられる（パッチアップデート）なら問題はないと思います。

sksat commented 6 months ago

これは，今 develop branch にマージされている（GNSS 関連の？）S2E 側ないしその新しい S2E 側のコードの挙動に依存した C2A user（存在不明）で，この PR をマージすることによって困ることがある，というように読み取れうるので，そんなことがあるのか？という質問だったのですが，そんなことがあるわけではなく，あくまで今 develop branch で major update を行っているので，今の develop branch のコードを使っている user があったら何も保証しようがないよね，というようなことですか？もしそのような意図であれば，それはそう以外のことはないです． > このパッチのマージ後に困ることがある

これは，この PR の base branch を main branch に差し替えて patch update とするのもよいのではないか，という提案ですか？もしそうであれば，それは非常にありがたいので，そのようにさせてもらえると助かります． > パッチが main に当てられるなら

200km commented 6 months ago

今の develop branch のコードを使っている user があったら何も保証しようがないよね，というようなことですか？

そうです。 developは一般ユーザー向けでないと明記しているのでそれで良いと思っていますが、このPRマージ後に何かしらのReleaseしてほしいという場合には、それはできないということです。 パッチアップデート以外のReleaseは今出ているPRがマージされてv8が出るまでできない状況です。

sksat commented 6 months ago

main をbase branch にして v7 に対するパッチアップデートにする，ということであれば可能である，ということだと読み取ったので，パッチをそのように修正しようと思います．それでよろしいですか？

200km commented 6 months ago

main をbase branch にして v7 に対するパッチアップデートにする，ということであれば可能である

修正前がmainから分岐しているならそれで問題ないと思います。今はdevから分岐してこの修正を行なっていると思うので、それでは想定と違うかなと思います。

sksat commented 6 months ago

PR の base branch を main にして main に rebase しました