-
Notifications
You must be signed in to change notification settings - Fork 3
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
BCのコピーをFRAMに取ることで不揮発化する #249
Conversation
これは @conjikidow くん担当の issue #22 と同じものなように見えます。 |
@conjikidow くんではなく自分が実装するという認識です。11月末まで日本を離れるので、実機検証はその後になると思います。 |
13bd593
to
0e78d12
Compare
遅くなりましたが、こちら一旦実装完了しました。ロジックの雰囲気や文法的なところなどレビューいただけるとありがたいです。 ちゃんと動作するかについては、実機でやってみないと分からない部分もあるので、今週のどこかで @conjikidow と実機検証したいと思います。 |
src/src_user/Applications/UserDefined/Cdh/non_volatile_bc_manager.c
Outdated
Show resolved
Hide resolved
src/src_user/Applications/UserDefined/Cdh/non_volatile_bc_manager.c
Outdated
Show resolved
Hide resolved
src/src_user/Applications/UserDefined/Cdh/non_volatile_bc_manager.h
Outdated
Show resolved
Hide resolved
src/src_user/Applications/UserDefined/Cdh/non_volatile_bc_manager.h
Outdated
Show resolved
Hide resolved
|
||
AppInfo APP_NVBC_MGR_create_app(void); | ||
|
||
CCP_CmdRet Cmd_APP_NVBC_MGR_SET_ENABLE(const CommonCmdPacket* packet); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[NITS] コマンド関数は作られていますが、コマンドDBには追加されていないようです。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
忘れていました。追加しました。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CDHコマンドの割り当て数が足りなくなったので、10から12に増やしました。その影響で、以降のコマンドIDがすべてずれています。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
わかりました。12だとギリギリなのであれば、もう少し増やして今後のためのマージンを持たせても良いかなと思います。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
15に増やしました
src/src_user/Applications/UserDefined/Cdh/non_volatile_bc_manager.c
Outdated
Show resolved
Hide resolved
src/src_user/Applications/UserDefined/Cdh/non_volatile_bc_manager.c
Outdated
Show resolved
Hide resolved
src/src_user/Applications/UserDefined/Cdh/non_volatile_bc_manager.c
Outdated
Show resolved
Hide resolved
src/src_user/Applications/UserDefined/Cdh/non_volatile_bc_manager.c
Outdated
Show resolved
Hide resolved
bc_id = begin_bc_id + i; | ||
nv_bc_manager_.is_ready_to_restore[bc_id] = 0; | ||
|
||
// 有効化されている BC のみコピーする |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Q] 有効化されていなくてもコピーしても良い気もするのですが、どうでしょうか?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 空き番のBCをコピーした場合、意味のないbyte列がFRAMにコピーされることになるが、それを間違えて復元して使おうとしたときに未定義の動作になってしまう可能性がある
- 無効化しているBCは理由があって無効化しているのであり、それを復元するのは、無効化している理由が引き継がれずそのまま使われる可能性があって危ない?
みたいなことを考えていました。実運用において、無効化したBCをコピーして復元したい場面があまり無いのであれば、このままで良いかなと思うのですが、どうでしょうか?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
わかりました。運用していく上で混乱が生じないように仕様やルールが共有されていればどちらの仕組みでも問題無いとは思います。
他でコメントしたものと関連して、これまで通りコマンドでBC追加した時、コマンドでBCをdisable/enableした時に、どうするべきか、何を機にすべきかなど整理してもらえると嬉しいです。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- コピーに関して
- アプリ有効化中は、8サイクル周期(10個ずつコピーする場合)でBCがFRAMにコピーされる
- 地上局からBLコマンドでBCを編集した際も、0.8秒後にはFRAMにコピーが取られることになる
- コマンドでBCを無効化すると、そのBCのready flagが下がるので、リセットしたときに復元できなくなる。再度BC有効化すると、復元できる
- モード遷移やEHで使う重要なBCは通常すべて有効化されているはずなので、問題ないと思われる
- 復元に関して
- AOBCがリセットしてしまったときは、デフォルトでアプリは無効化されている。復元しおえるまでは、アプリを有効化してはいけない
- Cmd_APP_NVM_BC_RESTORE_BC_FROM_NVM で一つずつ復元できる
- 無効化していたBCは復元できない
- 逆に言うと、復元できたBCはちゃんとしたBCなはずなので、不安なく使えるはず
- 所望のBC(打ち上げ時から定義変更したモード遷移やEHなど)を復元しおえたら、アプリを再度有効化する
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ざっとこんな感じだと思います。実機検証していろいろ分かってから、改めてソースコードやwikiに残したいと思います。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ありがとうございます。理解できました。
使い方をどこかに記載する際、次の点を考慮してもらえると嬉しいです。
- コピー、復元はどっち方向か分からない時もあるので、
NVMへのコピー
、NVMからの復元
など方向がわかるように書いてもらえると読みやすい - wikiに書く場合は誰でもwikiにアクセスできる仕組みが必要なので、公開方法も検討してほしい
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.h の先頭にコメント整備しました。
こちらの今回追加したことに関するwarningは対応してもらえると嬉しいです。 |
@chutaro 実装ありがとうございます。コメントつけさせていただきました。参考にしてください。 |
src/src_user/Applications/UserDefined/Cdh/non_volatile_memory_bc_manager.c
Outdated
Show resolved
Hide resolved
7266800
to
8399056
Compare
AOBC 単体SILSで検証できる部分は想定通り動作した。 |
@200km @conjikidow 実機検証を行い、想定通りの動作を確認できました!検証メモのリンクは概要欄に貼っています。改めてざっと確認していただき、良さそうであればマージしてしまいたいです。 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
小さなコメントだけなのでapproveしますが、その部分のコメントを修正してからマージしてください。
src/src_user/Settings/Modes/TaskLists/Elements/tl_elem_cdh_update.c
Outdated
Show resolved
Hide resolved
@chutaro 動作確認ありがとうございます。レビューしました。 |
Issue
詳細
詳しい実装方針は issue 参照
検証結果
ビルドチェック (どちらもチェック)
動作確認チェック (いずれかをチェック)
試験結果詳細記述場所 or 詳細ログ保存場所へのリンク