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.

Sign up for GitHub

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

Jump to bottom

Resize the driver receive buffer #161

Merged
merged 18 commits into from
Aug 8, 2023
Merged

Resize the driver receive buffer #161

merged 18 commits into from
Aug 8, 2023

Conversation

conjikidow
Copy link
Member

@conjikidow conjikidow commented Aug 6, 2023 •
edited
Loading

Issue

詳細

メモリの余裕を生むため，INAなどテレメ量が小さなものはバッファサイズを縮小し，逆にSTTなどテレメを増やしたいものは増やす。

検証結果

ビルドチェック (どちらもチェック)

  • SILSでのビルドチェックに通った(CIで確認)
  • vMicroでのビルドチェックに通った

動作確認チェック (いずれかをチェック)

  • SILSでアルゴリズムが想定通りに動いた
  • 実機でアルゴリズムが想定通りに動いた
  • (テレコマ試験の場合)コマンドファイルを使った試験をパスした

試験結果

image
テレメについてはMOBC以外をSILSで確認，MPUやRM，FRAMなどEMに接続されているコンポをEMで確認済み。

補足

STTのテレメについては #137 を参照のこと。
また，これのマージ後に #159 に対応する。

@conjikidow conjikidow added 🚀 priority::high priority high 🐬 minor update Minor update labels Aug 6, 2023
@conjikidow conjikidow added this to the 開発仮目標1 milestone Aug 6, 2023
@conjikidow conjikidow requested review from sksat and a team as code owners August 6, 2023 03:11
@conjikidow conjikidow self-assigned this Aug 6, 2023
@conjikidow
Copy link
Member Author

conjikidow commented Aug 6, 2023 •
edited
Loading

この方針に従い，とりあえずI2Cのコンポだけ修正しました。DS_IF_RX_BUFFER_SIZE はまだ修正していません。

この時点で，メモリ使用量は 122292byte (93.3%) から 116916byte (89.2%) になりました。
また，テレメが問題なく受信できることをEMで確認しました。

@conjikidow
Copy link
Member Author

@seki-hiro #137 に

ただし、histogram, centroids, matchedstarsは、それぞれdata部分のサイズが144, 146, 321であり、bufferが溢れています。

とありますが，321byte以上であれば問題ないということでしょうか。

@seki-hiro
Copy link
Member

Sagitta時刻情報、xxhashなどがついて20byte増え、エンコード前のフレームサイズは341byteあります。
さらに、SLIP Framing Protocolに従ってエンコードが行われ、header・footerと同じ文字列は2byteの文字列に置き換えられる（=Escape）ので、理論上は341 * 2 + 2(header + footer)の684byteになりうる可能性があります。

一方でEscapeがそんなに起きる可能性は低いので、理論上の最大分まで確保しなくても良いかもしれません。最低でも、341+2=343byteは必要になります。

@conjikidow
Copy link
Member Author

わかりました，ありがとうございます。
最低343byte，できれば684byte確保する方向で検討します。

@conjikidow
Copy link
Member Author

conjikidow commented Aug 6, 2023 •
edited
Loading

I2Cのみ縮小した状態で DS_IF_RX_BUFFER_SIZE を

  • 1024byte とするとメモリオーバーとなる
  • 684byte とすると 125760byte (95.9%) となる
  • 343byte とすると 119972byte (91.5%) となる

ことから，I2Cでないコンポについて単に DS_IF_RX_BUFFER_SIZE を大きくするという方針をとった場合，343byte なら可能です。

一方でメモリ使用量を減らすモチベーションはSTT以外にもある（#65 など）ため，それらとの兼ね合い次第ではUARTのコンポについても個別にバッファサイズを設定する必要があるのかなと思います。

@conjikidow conjikidow removed request for a team and sksat August 6, 2023 05:25
@conjikidow
Copy link
Member Author

conjikidow commented Aug 6, 2023 •
edited
Loading

一旦 DS_IF_RX_BUFFER_SIZE を 343byte として push しました。EMでの動作確認もできています。

@200km @seki-hiro
STIMなども減らすのが良いでしょうか。それともこのままで良いでしょうか。
STT以外を小さくすれば，STTについては684byteの確保も可能かと思います。

@200km 200km mentioned this pull request Aug 6, 2023
Merged
5 tasks
@200km
Copy link
Member

200km commented Aug 6, 2023

お疲れ様です。
メモリがギリギリなので、STIMやMOBCを減らしていくのもありだと思います。
もしくはこれは今の状態で一旦マージして、先に別のメモリ削減(https://github.com/ut-issl/c2a-aobc/issues/22)などに力を入れても良いかもしれません。

@conjikidow
Copy link
Member Author

STT（デフォルト値）を 350byte に増やし，STIM，OEM，MOBCについて個別に削減しました。
MOBCについては @.chutaro に確認した上で多めに 128byte，またFRAMについてはわからなかったため保留しデフォルト値のままとしています。

この状態で 118560byte (90.4%) となりました。
STTの試験もあるため，一度これでマージしたいと思います。
STTの試験後に，必要であれば別PRでSTTのサイズを増やしたりMOBCやFRAMのサイズを減らしたりしようと思います。

@conjikidow conjikidow changed the title WIP: Resize the driver receive buffer Resize the driver receive buffer Aug 7, 2023
@conjikidow
Copy link
Member Author

@200km @seki-hiro @chutaro レビューをお願いできますでしょうか。
テレメについてはメモリを調整したコンポのうち

  • MOBC以外をSILSで確認
  • MPUやRMなどEMに接続されているコンポをEMで確認

しておりますが，それ以外は確認できないため，マージ後にSTTの試験と合わせて実施できたらと思います。

@200km
Copy link
Member

200km commented Aug 7, 2023

@conjikidow FRAMは読み出しサイズを自由に調整できますが、最大読み出しサイズをここできめています。通信方式としてはSPIで同期式です。

c2a-aobc/src/src_user/Drivers/Cdh/fm25v10.h

Line 13 in 605722a

#define FM25V10_MAX_LENGTH (128) //!< 読み書きする最大データ数（実行速度などで決める）

@conjikidow
Copy link
Member Author

@200km それで良いのですね，ありがとうございます。
バッファサイズはそれと同じ128byteで良いのでしょうか。それとも少し大きくするなどが必要でしょうか。

@200km
Copy link
Member

200km commented Aug 7, 2023

バッファサイズはそれと同じ128byteで良いのでしょうか。

コードを読む限り良さそうに見えますが、試験してみてください。NVMやDRのOPSを参考にすればEMで試験できると思います。

@conjikidow
Copy link
Member Author

conjikidow commented Aug 7, 2023 •
edited
Loading

ありがとうございます。
FRAMについても128byteに縮小し，EMでの試験の結果問題なさそうです。
その結果 116272byte (88.7%) まで削減できました。

@200km
Copy link
Member

200km commented Aug 7, 2023

上のコメントでSTIMは16byteとありましたが、間違っていませんかね？

S2E側では最大テレメサイズが30と定義されています。デフォルトで使っている温度と角速度だけ出力するテレメも
温度:2byte * 3軸 = 6byte
角速度:3byte * 3軸 = 9byte
ヘッダフッタ:3byte
で合計18byteになっているような気がします。

STIMでは他のテレメモードの実装もおこなっており、コマンドによってより長いテレメトリを取得できるようにも変えることができます。そのコマンドを送ったときには最大で30byteになるということだと思います。

@conjikidow
Copy link
Member Author

ご指摘ありがとうございます。
16byteで押さえられるという意味で，データシートから最大13byteと認識していました。
しかし改めて確認したところ，モードの見落としがあり，最大22byteのようです。
多めに32byteに修正します。

200km
200km reviewed Aug 7, 2023
View reviewed changes
src/src_user/Drivers/Etc/mobc.h
@@ -12,6 +12,8 @@
#include <src_core/Drivers/Protocol/common_tlm_cmd_packet_for_driver_super.h>
#include <src_core/TlmCmd/packet_handler.h>

#define DS_IF_RX_BUFFER_SIZE_MOBC (128) //!< IF_RXのバッファサイズ
Copy link
Member

Choose a reason for hiding this comment

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

[IMO] MOBCから送られてくるデータは、GSからのコマンドに相当すると思います。
AOBCではブロックコマンドで使えるようにという観点から、1コマンドのサイズは下記の32byte以下になるように調整しています。

c2a-aobc/src/src_user/Settings/TlmCmd/block_command_table_params.h

Line 15 in bd8c8cb

#define BCT_CMD_MAX_LENGTH (32)

ブロックコマンドで使えなくてもよいコマンドでは下記の値が採用されると思いますが、現状使っているコマンドで32を超えているものはないように思います。coreコマンドで引数がrawのものはコマンドサイズを大きくできますが、本当に128が必要か考えても良いかもしれません。

https://github.com/ut-issl/c2a-core/blob/042cdfa15b0056880398e857cdd5d5a430562fd1/TlmCmd/Ccsds/space_packet_typedef.h#L12

あと、このあたりの関係性をメモとしてコメントに残しておくと良いかもしれません。

AOBCで、メモリ配分をどうを決めたかなどはこちらを参考にしてください。

Copy link
Member Author

@conjikidow conjikidow Aug 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

ありがとうございます。
どこまで削減できるかわからず，上に書いたようにCDHに確認した上で128byteであれば絶対に問題ないとのことから多めに128byteとしていました。

AOBCではブロックコマンドで使えるようにという観点から、1コマンドのサイズは下記の32byte以下になるように調整しています。

ブロックコマンドで使えなくてもよいコマンドでは下記の値が採用されると思いますが、現状使っているコマンドで32を超えているものはないように思います。

この辺りのことは知らなかったため，勉強になります。ありがとうございます。

Copy link
Member Author

Choose a reason for hiding this comment

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

ブロックコマンドで使えなくてもよいコマンドでは下記の値が採用されると思いますが、現状使っているコマンドで32を超えているものはないように思います

coreコマンドで引数がrawのものはコマンドサイズを大きくできます

これらを踏まえて 32byte に縮小しても問題ないということでしょうか。
引数がrawの場合，実用上32byteを超えることはないと思って大丈夫なのでしょうか。

Copy link
Member

Choose a reason for hiding this comment

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

コンポ数がひとつで影響も少ないので、とりあえずは128でもよいです。めもりを最適にしたいということになったらまたその時に考えましょう。

Copy link
Member Author

Choose a reason for hiding this comment

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

了解です。コメントだけ加えておきます。

src/src_user/Settings/DriverSuper/driver_super_params.h Outdated
@@ -11,7 +11,7 @@
#define DS_STREAM_MAX (1)

// TODO: 適切な値を考える
// OEM7600Driverに合わせて64から144に変更した
#define DS_IF_RX_BUFFER_SIZE (144)
// Sagittaに合わせて144から350に変更した
Copy link
Member

Choose a reason for hiding this comment

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

[NITS] 144からなどはgit logで残る情報なのでコメントに書く必要はないかなと思います。

Copy link
Member Author

@conjikidow conjikidow Aug 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

// OEM7600Driverに合わせて64から144に変更した

があったのでそのまま編集しましたが，おっしゃる通りなので変更履歴は削除します。

一方でこの値の根拠はあったほうが良いと思うので，コメント修正します。

@200km
Copy link
Member

200km commented Aug 7, 2023

最大22byteのようです。

一応間違いがないように訂正ですが、データシート上はCRLF付与モードにすると23byteになるようです。（P17 Table 5-12, 0起算で22までなので、合計23byte）

S2Eは余裕を持って３０としているようですね。キリの良い３２でも問題ないと思います。

@conjikidow
Copy link
Member Author

conjikidow commented Aug 7, 2023 •
edited
Loading

訂正ありがとうございます。

0起算で22までなので、合計23byte

0起算なのに1を足し忘れていましたね，何度もすみません。

@chutaro chutaro mentioned this pull request Aug 8, 2023
Merged
16 tasks
@200km 200km self-requested a review August 8, 2023 06:53
200km
200km approved these changes Aug 8, 2023
View reviewed changes
Copy link
Member

@200km 200km left a comment •
edited
Loading

Choose a reason for hiding this comment

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

修正されたのでapproveします。

@conjikidow
Copy link
Member Author

conjikidow commented Aug 8, 2023 •
edited
Loading

ありがとうございます。

最終的に 116304byte (88.7%) となりました。
approve後の修正はコメントの追加だけなので，そのままマージします。

@conjikidow conjikidow merged commit f8f9665 into develop Aug 8, 2023
@conjikidow conjikidow deleted the feature/resize_di_rx_buffer branch August 8, 2023 07:42
@conjikidow conjikidow mentioned this pull request Aug 8, 2023
Closed
@conjikidow conjikidow linked an issue Aug 8, 2023 that may be closed by this pull request
Driver受信バッファサイズを調整する #39
Closed
@conjikidow conjikidow removed a link to an issue Aug 8, 2023
Driver受信バッファサイズを調整する #39
Closed
@seki-hiro seki-hiro mentioned this pull request Aug 14, 2023
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@200km 200km 200km approved these changes

@chutaro chutaro Awaiting requested review from chutaro

@seki-hiro seki-hiro Awaiting requested review from seki-hiro

@suzuki-toshihir0 suzuki-toshihir0 Awaiting requested review from suzuki-toshihir0

@t-hosonuma t-hosonuma Awaiting requested review from t-hosonuma

Assignees

@conjikidow conjikidow

Labels
🐬 minor update Minor update 🚀 priority::high priority high
Projects
None yet
Milestone
開発仮目標1
Development

Successfully merging this pull request may close these issues.
3 participants
@conjikidow @seki-hiro @200km