Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add C2A command sender for accelerated SILS test #485

Merged
merged 35 commits into from
Oct 5, 2023

Conversation

200km
Copy link
Member

@200km 200km commented Sep 1, 2023

Overview

Add C2A command sender for accelerated SILS test

Issue

Details

I implemented C2A command sender component for accelerated SILS test.
Users can use WINGS's ops file to define the commands they want to send.

Validation results

I confirmed that the functions work well with S2E-AOBC

Scope of influence

Minor update

Supplement

NA

Note

NA

@200km 200km added priority::medium priority medium C2A Something related with C2A flight software WINGS Something related with WINGS ground station component component emulation labels Sep 1, 2023
@200km 200km added this to the Major Update for v7.0.0 milestone Sep 1, 2023
@200km 200km self-assigned this Sep 1, 2023
@200km 200km requested review from sksat and a team as code owners September 1, 2023 13:34
@200km 200km requested review from suzuki-toshihir0 and removed request for a team September 1, 2023 13:34
Copy link
Collaborator

@sksat sksat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

これは意味的にだいぶ巨大な変更ですね......

なんだかんだであんまり明示できていなかったのですが,C2A と S2E は基本的にできるだけ疎結合にしたいと思っていて,S2E の ObcWithC2a 以外の部分で C2A の機能が用いられることは考えていませんでした.また,この部分ですら TMGR_count_up_master_clock() のような C2A 内部の関数を直接呼んでいることはよくないと考えており,これをさらにラップした C2A_core_tick() のような関数を用意して,S2E などのランタイムにはそれを呼んでもらうように変えたい,といった計画をしています.

そのため,使う C2A の機能を増やすのであればそれは慎重に考えたいです.
それでいうと,ENDIAN_memcpy() などは必ずしも必要ではない(別に SILS-S2E はビッグエンディアンな環境でビルドすることはなさそう)ですし,あまり使ってほしくないですね......

今回の PR の場合CCP_register_rtc(), CCP_register_tlc() などは本質的に必要になるので仕方ないと思うのですが,これはどうにかして明示したいですね.

@sksat
Copy link
Collaborator

sksat commented Sep 4, 2023

必ずしも ops ファイルの全機能をサポートするわけではない(しようと思っても現状では実装が仕様と化しているので厳しい)と思うので,WINGS の ops ファイルが使える,というような表現は避けて,あくまでサブセットであると明示した方がよいと思います

@200km
Copy link
Member Author

200km commented Sep 4, 2023

C2A と S2E は基本的にできるだけ疎結合にしたい

大きな流れとしてそうしていきたいのは、これまでの議論から察していますし、私も同意です。
また、今回の機能の実現方法として今のS2E-C2A-WINGS SILS形態でWINGS側が高速で動くように修正するなど、C2AとS2Eの結合をこれ以上高めない方法などもあるとは思っています。
ただ、それらの実装がなかなか進まないなか、S2E-AOBC関連のユーザーが増えてきたので「高速SILSでコマンドを送る」機能の必要性が高まっています。この要望にクイックに応えるには、他の実装を待つのではなくS2E側で実装できる今回のような形が良いと思っています。

将来的にC2AとS2Eの結合が解けるようなことが進んだ場合には、この機能は無くしてしまって良いとは思います。

ENDIAN_memcpy

こちらは、使わない方向で考えてみます。

明示したい

言葉がよくわかっておらず申し訳ないですが、どのようなことをすれば明示したということになるのでしょうか?.
コード中のコメントなどで記述するという感じなのか、s2e-document的なものに書くべきなのかなど教えてください.
例えばOPSファイルについてはサンプルにサポートしていない機能などは書いていますが、別の場所にも分かりやすく書くべきということでしょうか?

@200km 200km added the minor update add functionality in a backwards compatible manner label Sep 4, 2023
@200km
Copy link
Member Author

200km commented Sep 4, 2023

C2AのENDIAN_memcpyではなく、S2Eが持っていたendian_memcpyを使うようにしました。

@sksat
Copy link
Collaborator

sksat commented Sep 7, 2023

高速で SILS を回しつつコマンドを送りたい,という要求自体は AE 側でも上がっていて,その重要性についてはとても同意します(pytest を高速化したい,など).
その上で,直近でこの要望にクイックに応えるには S2E 側で今回のような実装をする,ということもとてもアリだと思っています.
ただし,S2E <-> C2A の結合を密にしてしまう,ops ファイル・TlmCmdDB の実装(のサブセット)が増えてしまうといった点から,この機能をこの実装方針のまま維持しようとすると大きな負債となってしまうのではないかということを危惧していました.そのため,

将来的にC2AとS2Eの結合が解けるようなことが進んだ場合には、この機能は無くしてしまって良いとは思います。

ということであれば,その懸念はある程度解消できるかな,と思います.
また,他の実装,については Gaia が OSS 化されたこともあり,S2E-C2A-Gaia での SILS が可能になれば比較的すぐ実装可能なのではないか,とも考えています(Gaia では1秒おきの更新ではなくストリーム接続なので,高速な動作に耐えられます).

ありがとうございます. > ENDIAN_memcpy を使わないように実装

すみません言葉不足でした.これはどちらかというと C2A(-core)側の話で,C2A(-core) が S2E を含む外部のソフトウェア(ランタイム/シミュレータ)に対して公開しなければならない関数(単に仕組み的なところでは include してしまえばなんでも使えてしまいますが,それでは疎結合にできないので使っていい関数をホワイトリスト方式で管理したい)をドキュメントなどで明らかにして,そのインターフェースを変更する場合は major update としていきたい,という意図でした.とはいえ S2E 側でもドキュメント(ユーザに対するもの,というよりはコーディングルールの類ですかね)で C2A のこれは使ってよい,というようなことは明示しておきたいかもですね. > 明示したい

@200km
Copy link
Member Author

200km commented Sep 7, 2023

@sksat ありがとうございます。

Gaia が OSS 化されたこともあり,S2E-C2A-Gaia での SILS が可能になれば比較的すぐ実装可能

これは、ありがたい情報ですね。私自身はGaiaはまだ使ったことがない状況ですので、まずはリアルタイムのS2E-C2A-Gaia SILS環境を試してみたいと思います。もし手順書などすでにあれば教えてもらえると嬉しいです。

明示する

説明理解しました。外部に対するAPIをきちんと定義するというようなことにも関係しますかね。(S2Eもsemver使っているのにAPI定義していないのでいつかちゃんとしなきゃなとは思っています。)

とりあえずこのPRではどうすれば良いでしょうか?

@sksat
Copy link
Collaborator

sksat commented Sep 7, 2023

S2E と連携させるのはまだやっていないのですが,c2a-sils-runtime・C2A での SILS(c2a-runtime,と各所で呼んでいたもの)での pytest などはできるようになっています(pytest CI も組んでいます).ドキュメントはまだあまり整備できていないのですが.
pytest: https://github.com/arkedge/c2a-core/tree/develop/examples/mobc/src/src_user/Test
デバッグ用のコマンド送信・テレメ表示ツール: https://github.com/arkedge/c2a-devtools

Gaia

c2a-core v4 では(そもそも API 定義を明確に行おうとしている他に) CHANGELOG.md という変更履歴を書くためのファイルを作って,一旦そこで breaking change(ないし API 定義して breaking ということにする変更)の外部に対する説明をするようにしよう,といったことをやろうとしたりしていますね.

とりあえずこの PR では,C2A の機能を使っているソースファイルの上の方で使っている構造体・関数を箇条書きでコメントとして書いておく,のような対応とするのではいかがでしょうか?その上で,s2e-documents などでこの機能を紹介する際にはまだこの機能は experimental/optional なものであると明記してもらう,などがよい気がします.

@200km
Copy link
Member Author

200km commented Sep 11, 2023

@sksat ありがとうございます。コメントつけましたのでレビューお願いします。

@sksat
Copy link
Collaborator

sksat commented Oct 5, 2023

修正ありがとうございます.c2a-core v4 まわりでもう一点修正お願いします.他は問題なさそうです.

@sksat
Copy link
Collaborator

sksat commented Oct 5, 2023

あと,9c9369c を見て思ったんですが,C2A_CORE_VER_MINOR >= 9` みたいなバージョンチェックは入れてもいいかもです.

@200km
Copy link
Member Author

200km commented Oct 5, 2023

C2A_CORE_VER_MINOR >= 9` みたいなバージョンチェック

9c9369の修正は、version 9以上でしか使えなくしたというわけではなく、「Cmd_ prefixがついているかのversionは関係なく、Cmd_DB上のコマンド表記とOPSファイル内のコマンド表記が一致しておけば良い」という修正になります。
C2Aが古く、Cmd_DB上でCmd_ prefixがついたものであれば、そのまま使えます。

@sksat
Copy link
Collaborator

sksat commented Oct 5, 2023

なるほどです.勘違いしていました > Cmd_ prefix

@200km 200km requested a review from sksat October 5, 2023 18:23
@200km 200km merged commit 0b29557 into develop Oct 5, 2023
11 checks passed
@200km 200km deleted the feature/add-c2a-command-handling-feature branch October 5, 2023 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C2A Something related with C2A flight software component component emulation minor update add functionality in a backwards compatible manner priority::medium priority medium WINGS Something related with WINGS ground station
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

高速SILSでC2Aにコマンドを送られるシステムを作る
3 participants