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

DRのないOBCで,不要なメモリを確保しないようにする #264

Merged
merged 4 commits into from
Mar 7, 2022

Conversation

meltingrabbit
Copy link
Collaborator

@meltingrabbit meltingrabbit commented Mar 1, 2022

概要

DRのないOBCで,不要なメモリを確保しないようにする

Issue

NA

詳細

stored tlmとreploay tlmのpacket listを,DRのないボードではメモリ確保しないようにした

検証結果

@meltingrabbit meltingrabbit requested review from chutaro and yngyu March 1, 2022 08:04
@meltingrabbit meltingrabbit self-assigned this Mar 1, 2022
@meltingrabbit meltingrabbit added the priority::medium priority medium label Mar 1, 2022
@yngyu
Copy link
Contributor

yngyu commented Mar 2, 2022

[IMO] ほとんどが ifndef + DISABlE で 2重否定になっているので、デフォルトとは言え #define PH_USE_DATA_RECORDER の方がわかりやすい気がするのですがどうでしょう?

@meltingrabbit
Copy link
Collaborator Author

どっちがいいか悩んでました.

結局,

  • DR使いたい人(OBC)が define する,なら PH_USE_DATA_RECORDER
  • DR使わない人(OBC)が define する,なら PH_DISABLE_DATA_RECORDER

だと思っていて,どっちが標準なん?となり,ま~あるのが標準やろ~~~となって今にしてました.

どっちがいいかね?

Copy link
Contributor

@yngyu yngyu left a comment

Choose a reason for hiding this comment

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

あるのが標準ですが、#ifndef DISABLE_DATA_RECORDER はちょっと紛らわしく無いでしょうか?混乱しそうで
@chutaro どっちが良いと思う?

@meltingrabbit
Copy link
Collaborator Author

紛らわしく無いでしょうか?混乱しそうで

うん,紛らわしい.

@meltingrabbit
Copy link
Collaborator Author

ぐちょくに PH_USE_DATA_RECORDER にするか~

ってのと,もはやこれ PH_ にすべきじゃない?

今後,DRをcoreに入れた時,全体をこのifdefでくるむことになると思うんだよね.

となると,このdefはどこに置くのが良いんだ?

@meltingrabbit
Copy link
Collaborator Author

src/src_user/Settings/Applications/data_recorder_params.h

@yngyu
Copy link
Contributor

yngyu commented Mar 3, 2022

src/src_user/Settings/Applications/data_recorder_params.h ?

で良いと思います

#define USE_DATA_RECORDER
#ifdef USE_DATA_RECORDER
#define DRのパラメーター
~
#endif

とか?

@meltingrabbit
Copy link
Collaborator Author

なんか,これのためだけに data_recorder_params.h つくるの渋いなぁ..と思ったりもしたけど,まあそうよね.

USE_HOGE 系まとめてもいいかな?って思ったけど,ほかはビルド対象にしないだけだし,他と密結合しうるのはDRぐらいだよね.

@meltingrabbit
Copy link
Collaborator Author

接頭辞考えて, DR_USE_DATA_RECORDER とかで直しておきます.

@meltingrabbit meltingrabbit force-pushed the feature/select_dr_enable branch from a8eb378 to 5aa0319 Compare March 4, 2022 13:35
@meltingrabbit
Copy link
Collaborator Author

直しました.

@meltingrabbit meltingrabbit force-pushed the feature/select_dr_enable branch 2 times, most recently from d6be989 to f6a603c Compare March 4, 2022 15:12
@meltingrabbit meltingrabbit requested a review from yngyu March 4, 2022 15:18
Comment on lines 293 to 305
static PH_ACK PH_add_st_tlm_(const CommonTlmPacket* packet)
{
#ifdef DR_ENABLE
PL_ACK ack = PL_push_back(&PH_st_tlm_list, packet);

if (ack != PL_SUCCESS) return PH_ACK_PL_LIST_FULL;

return PH_ACK_SUCCESS;
#else
(void)packet;
return PH_ACK_SUCCESS;
#endif
}
Copy link
Contributor

Choose a reason for hiding this comment

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

[IMO] この関数まるごと #ifdef #endif で良いと思います

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

了解です.

それに伴い,

fbc5132

と,関連部分も変えてます.

@meltingrabbit meltingrabbit force-pushed the feature/select_dr_enable branch from f6a603c to 7f379e7 Compare March 6, 2022 06:45
@meltingrabbit meltingrabbit requested a review from yngyu March 6, 2022 06:49
@meltingrabbit meltingrabbit merged commit d41fdec into develop Mar 7, 2022
@meltingrabbit meltingrabbit deleted the feature/select_dr_enable branch March 7, 2022 04:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority::medium priority medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants