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

DS での rx_buffer を stream ごとに driver instance から設定可能にする #503

Merged
merged 6 commits into from
Mar 4, 2023

Conversation

meltingrabbit
Copy link
Collaborator

@meltingrabbit meltingrabbit commented Feb 28, 2023

概要

DS での rx_buffer を stream ごとに driver instance から設定可能にする

Issue

詳細

Driverが持っていた DS の rx buffer を DI で生成し,Driver に渡すようにした.
これによって,同一 Driver で複数の DI を持つコンポに対応

検証結果

既存のテストがすべて通る

影響範囲

DI,Driverの初期化がすべて変わる

備考

@meltingrabbit meltingrabbit added enhancement New feature or request priority::high priorityg high labels Feb 28, 2023
Base automatically changed from feature/ds_fix_comment to develop March 1, 2023 06:27
@meltingrabbit meltingrabbit force-pushed the feature/stream_buffer_allocation_in_di branch from 40d3384 to 5ca3b36 Compare March 1, 2023 06:28
@meltingrabbit meltingrabbit force-pushed the feature/stream_buffer_allocation_in_di branch from a770f4d to 76a2e02 Compare March 1, 2023 18:19
@meltingrabbit meltingrabbit changed the title WIP: DS での rx_buffer を stream ごとに driver instance から設定可能にする DS での rx_buffer を stream ごとに driver instance から設定可能にする Mar 1, 2023
@meltingrabbit meltingrabbit changed the base branch from develop to feature/result_enum March 1, 2023 18:26
@meltingrabbit
Copy link
Collaborator Author

meltingrabbit commented Mar 1, 2023

@chutaro @ToshiAki64 レビューお願いします.
これで #443 に続く改修がようやく落ち着きます.
v3.8.0 で,Cmd返り値変更に並ぶ重大な update になりそうです.

この PR のあと,細かい DS 周りの修正を予定しています.

@meltingrabbit meltingrabbit requested a review from flap1 March 1, 2023 18:37
Base automatically changed from feature/result_enum to develop March 4, 2023 06:16
@chutaro
Copy link
Contributor

chutaro commented Mar 4, 2023

reviewというより、DSに対する質問いくつか書いていってもいいですか?

  • stream 0 しか使わない大半のコンポでも、Driver構造体を確保した時点で DS_StreamConfig 構造体は3つ確保されてしまって、2つは使わないからメモリの無駄になってしまうと思うんですが、DriverSuper構造体のメンバーとして DS_StreamConfig もポインターで保持して、使う分だけ DI で確保する、という方針にはならなかったのでしょうか?
  • 一方で、DS_StreamConfig 構造体の中で一番メモリを使うであろう DS_StreamRecBuffer については、ポインターで確保していて、stream 0 しか使わない場合は 1,2 は NULL埋めするので、まあ問題ない、という感じですかね?
  • tx_frame_ は Driverで確保して、 rx_buffer は DI で確保するのは何故なんでしたっけ?
  • これはただのコメントなのですが、rx_bufferを可変にする改修は既に終わってるんですね(あまり意識してませんでしたありがとうございます)。今回はその確保場所を DI に移して色々整理した、という感じですよね?

@meltingrabbit meltingrabbit force-pushed the feature/stream_buffer_allocation_in_di branch from 2180a2f to 4bceb79 Compare March 4, 2023 06:18
@meltingrabbit
Copy link
Collaborator Author

meltingrabbit commented Mar 4, 2023

Driver構造体を確保した時点で DS_StreamConfig 構造体は3つ確保されてしまって、2つは使わないからメモリの無駄になってしまうと思うんですが、DriverSuper構造体のメンバーとして DS_StreamConfig もポインターで保持して、使う分だけ DI で確保する、という方針にはならなかったのでしょうか?

これが今までメモリ使用量が多いい問題の元凶だったので,一番メモリを使っているバッファ(= stream buffer)だけ,使うstreamごとに外部からわたせるように #503 にした感じです.現在の DS_StreamConfig 構造体は軽いので,許容かなという(C でかつ,malloc 禁止な C2A だと,多分これがメモリと安全性のちょうどいいバランスだと思ってます)

一方で、DS_StreamConfig 構造体の中で一番メモリを使うであろう DS_StreamRecBuffer については、ポインターで確保していて、stream 0 しか使わない場合は 1,2 は NULL埋めするので、まあ問題ない、という感じですかね?

そそ

@meltingrabbit
Copy link
Collaborator Author

meltingrabbit commented Mar 4, 2023

tx_frame_ は Driverで確保して、 rx_buffer は DI で確保するのは何故なんでしたっけ?

  • tx_frame は送信側なのでframe解析がない
  • 送信するときに driver 側で作って,送信後は不要になるので,他の DI で共通化可能(DI ごとにつくるとむしろ無駄)
    • rx は途中まで受信してるときなど,次回の rec 呼び出しに繰り越さないと行けないので,共用不能
  • 以下の理由で tx は rx よりそもそもシンプルでこれで十分
    • 受信は,rec呼び出し頻度や物理バッファによって,一度に確保すべきメモリが受信frameよりも大きく,きちんと確保することが大事
    • 送信は,frame 解析が不要で,普通に送るバイナリ列を上から作ってくだけの処理

って感じかな.
あと,tx はコマンドで, rx はテレメであり, tx の frame 長 は一般的に小さい,ってのもある.

@meltingrabbit
Copy link
Collaborator Author

meltingrabbit commented Mar 4, 2023

これはただのコメントなのですが、rx_bufferを可変にする改修は既に終わってるんですね(あまり意識してませんでしたありがとうございます)。今回はその確保場所を DI に移して色々整理した、という感じですよね?

そそ.実はね.
ここらへんは @yngyu くんレビューしてくれてた.

このあたり

@chutaro
Copy link
Contributor

chutaro commented Mar 4, 2023

なるほどです。全部納得しましたありがとうございます。

Copy link
Contributor

@chutaro chutaro left a comment

Choose a reason for hiding this comment

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

良さそうです。いくつか些細なコメントいれています

Copy link
Contributor

@chutaro chutaro left a comment

Choose a reason for hiding this comment

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

おつかれさまです!approveしました

@meltingrabbit meltingrabbit merged commit 92ef862 into develop Mar 4, 2023
@meltingrabbit meltingrabbit deleted the feature/stream_buffer_allocation_in_di branch March 4, 2023 07:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request priority::high priorityg high
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants