-
Notifications
You must be signed in to change notification settings - Fork 10
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
support linux for uart and ccsds on SILS #341
base: develop
Are you sure you want to change the base?
Conversation
Examples/minimum_user_for_s2e/src/src_user/IfWrapper/Sils/sils_sci_if.cpp
Outdated
Show resolved
Hide resolved
Examples/minimum_user_for_s2e/src/src_user/IfWrapper/Sils/sils_sci_if.cpp
Outdated
Show resolved
Hide resolved
Examples/minimum_user_for_s2e/src/src_user/IfWrapper/Sils/sils_sci_if.cpp
Outdated
Show resolved
Hide resolved
Examples/minimum_user_for_s2e/src/src_user/IfWrapper/Sils/ccsds_sils_sci_if.hpp
Outdated
Show resolved
Hide resolved
Examples/minimum_user_for_s2e/src/src_user/IfWrapper/Sils/ccsds_sils_sci_if.hpp
Outdated
Show resolved
Hide resolved
Examples/minimum_user_for_s2e/src/src_user/IfWrapper/Sils/uart_sils_sci_if.hpp
Outdated
Show resolved
Hide resolved
Examples/minimum_user_for_s2e/src/src_user/IfWrapper/Sils/uart_sils_sci_if.hpp
Outdated
Show resolved
Hide resolved
Examples/minimum_user_for_s2e/src/src_user/IfWrapper/Sils/sils_sci_if.hpp
Outdated
Show resolved
Hide resolved
C用のフォーマットチェックがC++に刺さっとるな |
あーそういうことか.普段ここでC++書かれることないから... |
Allmanスタイルの場合, 流石に改行するか. |
まあ,最悪 https://github.com/ut-issl/c2a-core/blob/develop/Script/CI/check_coding_rule.json に追記してもらえれば大丈夫そう. |
81fe7a7
to
7496882
Compare
Examples/minimum_user_for_s2e/src/src_user/IfWrapper/Sils/sils_sci_if.cpp
Outdated
Show resolved
Hide resolved
ロジック変わってなくても,コード自体は変わってるので,windowsでもpytestをぜんぶ走らせ直しておいてもらえると 🙏 |
Examples/minimum_user_for_s2e/src/src_user/IfWrapper/Sils/ccsds_sils_sci_if.hpp
Outdated
Show resolved
Hide resolved
Examples/minimum_user_for_s2e/src/src_user/IfWrapper/Sils/sils_sci_if.cpp
Outdated
Show resolved
Hide resolved
7496882
to
40e386a
Compare
Examples/minimum_user_for_s2e/src/src_user/IfWrapper/Sils/ccsds_sils_sci_if.hpp
Outdated
Show resolved
Hide resolved
Examples/minimum_user_for_s2e/src/src_user/IfWrapper/Sils/ccsds_sils_sci_if.cpp
Outdated
Show resolved
Hide resolved
Examples/minimum_user_for_s2e/src/src_user/IfWrapper/Sils/sils_sci_if.cpp
Outdated
Show resolved
Hide resolved
400411a
to
c0ffc1e
Compare
該当ファイルをcheck_coding_rule.jsonに加えましたが弾かれました |
d01c6f4
to
7bddf76
Compare
check_encodingを解決しました |
@@ -3,116 +3,24 @@ | |||
* @file | |||
* @brief ccsds_sils_sci_if | |||
* @details WINGS TMTC IFとCCSDSのTransfer FrameをSCI COMでやりとりするIF | |||
Windows上でcom0comを使うことを想定 |
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.
[IMO] これは残しておいた方がいい?
Windowsでは
,とか
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.
残した上で一行追加しました
Examples/minimum_user_for_s2e/src/src_user/IfWrapper/Sils/ccsds_sils_sci_if.hpp
Outdated
Show resolved
Hide resolved
Examples/minimum_user_for_s2e/src/src_user/IfWrapper/Sils/sils_sci_if.cpp
Outdated
Show resolved
Hide resolved
@chutaro @yngyu @meltingrabbit |
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.
良さそうです.お疲れさまでした!
@meltingrabbit いちおう最後に見て approve お願いします |
これ,コード規約どうするか~ まあ, C++ のコードなので,無視してもいいんだけど,若干気になるよね. C2A の規約に合わせるなら,
なんだけど,どうしよう? @chutaro |
int myHComPort_; | ||
struct termios config_; | ||
#endif | ||
bool init_success; |
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.
[IMO] これも bool だし,
init_succeeded
とかのほうが良さそう.
このコード,かなり古いコードなので,ついでに色々直したい)
まあ,上のコメントとあわせて,命名規則修正などは別 PR でやる,ってのも OK です.
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.
このPR長引きすぎてるので、これは一回マージしてコード規約は別PR、でどうでしょう?
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.
じゃ,そうしましょうか.
issue立てとくね
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.
@flap1 あと最新のdevelop からrebaseしといてください! |
あ~,これ 2nd obc の方直ってないなぁ....直すか~ |
71c74dc で最低限の命名直してます. |
d8778b9 で 2nd obc 対応してます. ざっと見てもらえると |
5d0f3de
to
d8778b9
Compare
27c97bb
to
00bdfb1
Compare
最新のdevelopからrebaseしました。 |
2nd obcのテストって回してる?回してたらok! |
本当に遅くなってすいません、2nd obc の Test が Windows で通ることを確認しました、 Linux でのテストはまた時間がかかりそうなので別 issue 切るとして 一旦マージしてもよろしいでしょうか...? |
00bdfb1
to
66ef89b
Compare
rebase してくれてそう? |
概要
Examples/minimum_user_for_s2e/src/src_user/IfWrapper/Sils/sils_sci_if.hpp
でport通信の基底クラスSCIComPortを定義Issue
詳細
WIN32
を用いている)のでVSを用いた実行結果に影響はないsils_sci_if.hpp
でクラスSCIComPort
を定義,ccsds_sils_sci_if.hpp
とuart_sils_sci_if.hpp
のそれぞれで継承している(中身は同じなので継承させる必要はないかもしれない)検証結果
影響範囲
補足
注意