Merged
Conversation
takuchi17
requested changes
May 20, 2025
Collaborator
takuchi17
left a comment
takuchi17
left a comment
There was a problem hiding this comment.
一周見た感じ，細かいところばっかり．実際に動いているところを見たし，コード観た感じも致命的なところは無さそう．
全体で気になったのは以下．
- ファイル名が
Controllerでいいのかは気になる．個人的には何を制御するクラスなのかが分かった方がいい．MotorControllerとか． - モーターとモータが混在してるからどっちかに統一で．
aridome222
requested changes
May 20, 2025
Contributor
aridome222
left a comment
aridome222
left a comment
There was a problem hiding this comment.
細かいところだけちょっと回答お願いします。
Collaborator
takahashitom
left a comment
takahashitom
left a comment
There was a problem hiding this comment.
いったん、メンバ変数に関する話だけして、他の部分のレビューはしていないのでコメントです。
メンバ変数の消去に関しては、他の人の意見も聞きたいところです。
よろしくお願いします。
takuchi17
reviewed
May 20, 2025
takuchi17
requested changes
May 21, 2025
aridome222
approved these changes
May 21, 2025
Contributor
aridome222
left a comment
aridome222
left a comment
There was a problem hiding this comment.
setPowerの修正してくれてありがとう！お疲れ様！
molpui0726
approved these changes
May 21, 2025
Contributor
molpui0726
left a comment
molpui0726
left a comment
There was a problem hiding this comment.
僕が出したところは全部いい気がするので、いっっっったんLGTM
takuchi17
approved these changes
May 22, 2025
Collaborator
takuchi17
left a comment
takuchi17
left a comment
There was a problem hiding this comment.
自分が指摘した所は修正できてそうです．
かなりやること多かったと思うけど，よく書けていると思います．
実際に動くとこも見たし，さほど心配はしてないかな．動かしたところ見たときは速度を指定して途中で違う速度に設定してみたりをしていないから，一応確認はしておきたいですね．具体的には，昨年はモーターインスタンスを何回も生成してセットアップしていてフリーズやカクカク走行になっていたから，今回はどうなのかを，次イテレーションで確認したい．
一旦お疲れありがとうLGTM!!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
チェックリスト
変更点
modules/API/Motor.hを追加modules/API/Port.hを追加modules/API/Controller.h,modules/API/Controller.cppを追加Makefile.incを変更CMakeLists.txtを変更tests/ControllerTest.cppを追加tests/dummy/pbio/port.h,tests/dummy/spike/pup/motor.h,tests/dummy/spikeapi.hを追加動作テスト
Notionを参考
メモ
modules/API/Motor.hのisStalled(),setDutyLimit(),restoreDutyLimit(),はControllerクラスでは対応していません。必要に応じて今後追加していきます。-I$(mkfile_path)../../common/library/libcpp-spike/includeを追記することで、このディレクトリ内にあるspikeapiラッパクラス群を参照可能。これを用いる場合は、modules/API/Motor.h,modules/API/Port.hは不要。