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.

Sign up for GitHub

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

Jump to bottom

checkDataFormat() and unit test updates #54

Merged
merged 13 commits into from
Aug 6, 2024
Merged

checkDataFormat() and unit test updates #54

merged 13 commits into from
Aug 6, 2024

Conversation

YusukeKato
Copy link
Contributor

@YusukeKato YusukeKato commented Apr 10, 2024 •
edited

What does this implement/fix?

checkDataFormat関数を下記のように更新しました。

  • ASCII形式のチェック機能を追加
  • BinaryでもASCIIでもない形式のチェックも追加
  • while文を追加&タイムアウト機能を追加

それ以外にも下記を修正しました。

  • Binary形式のチェックを強化
  • 不正なデータを読み込んでから正常なデータを読み込むように単体テストを修正
  • 必要がなくなったのでhasCompletedFormatCheck()を削除

Does this close any currently open issues?

下記のissueに対応します。

How has this been tested?

実機とテストの両方で動作確認をしました。

Any other comments?

  • Add unit tests #52 (comment) でヘルパー関数を作成する話がありましたが、実装に時間がかかりそうなので今回は見送りました。

Checklists

@YusukeKato YusukeKato changed the title Update checkDataFormat() to enable ASCII format check Update checkDataFormat() to add ASCII format check Apr 10, 2024
@YusukeKato YusukeKato requested a review from ShotaAk April 12, 2024 04:39
@YusukeKato YusukeKato self-assigned this Apr 12, 2024
@YusukeKato YusukeKato added the Type: Feature New Feature label Apr 12, 2024
@YusukeKato YusukeKato marked this pull request as ready for review April 12, 2024 04:40
@YusukeKato YusukeKato changed the title Update checkDataFormat() to add ASCII format check checkDataFormat() and unit test updates Apr 12, 2024
@YusukeKato
Copy link
Contributor Author

checkDataFormat()のwhile文でsleep(10ms)を実行していましたが、sleepがなくても正常に動作することが確認できたので削除しました。

それに伴って、概要欄に記載していた下記の文章も削除しました。

実機のIMUからデータを取得する際、while文が高速にループしているとread()に失敗するため、sleep処理（10ms）を追加しました。

ShotaAk
ShotaAk requested changes Apr 17, 2024
View reviewed changes
Copy link
Collaborator

@ShotaAk ShotaAk left a comment

Choose a reason for hiding this comment

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

実装内容はほとんど良いと思います。

書き方についてコメントしました。確認お願いします。

test/test_driver.cpp Outdated Show resolved Hide resolved
src/rt_usb_9axisimu_driver_component.cpp Outdated Show resolved Hide resolved
src/rt_usb_9axisimu_driver.cpp Outdated Show resolved Hide resolved
src/rt_usb_9axisimu_driver.cpp Outdated Show resolved Hide resolved
src/rt_usb_9axisimu_driver.cpp Outdated Show resolved Hide resolved
src/rt_usb_9axisimu_driver.cpp Outdated Show resolved Hide resolved
@YusukeKato
Copy link
Contributor Author

YusukeKato commented Aug 2, 2024 •
edited

ここまでのレビューコメントに対応しました。
実機でも動作確認できました（Binary、ASCIIどちらも確認済み）。

@YusukeKato
Copy link
Contributor Author

YusukeKato commented Aug 5, 2024 •
edited

概要

ASCIIデータの読み込み時にASCII形式の判定に失敗する場合があったため、修正を行いました。
実機でもASCIIとBinaryのデータ、どちらでもデータが読み込めることを確認しました。

原因

BinaryとASCIIのデータが全て揃っている状態でないとデータ形式を判定できないが、従来の実装方法だとタイミングによってデータが欠ける可能性があったため。

修正内容

readFromDevice()でIMUからデータを取得する際、一度で1セット分のデータ全てを取得できないようだったので（下記参考）、取得したデータを貯められるように変更しました。
Binary形式の判定では貯めたデータ内の中から「0xff」を見つけて、そこを基準に判定します。
ASCII形式の判定では貯めたデータ内の改行文字から次の改行文字までの間のデータを見て判定します。
また、貯めたデータ全体を調べて判定するように実装を変更しています（貯めたデータの中から一箇所でもBinaryかASCIIに一致するデータがあれば判定が成功するような実装になっています）。

ASCIIデータ出力時のreadFromDevice()のログ

readFromDevice()実行1回目↓

read_size: 30
25,0.003193,0.000000,0.000000,

readFromDevice()実行2回目↓

read_size: 29
0.011230,-0.752930,-0.669922,

readFromDevice()実行3回目↓

read_size: 46
-316.200012,-594.599976,-201.899994,40.849045

ShotaAk
ShotaAk requested changes Aug 6, 2024
View reviewed changes
Copy link
Collaborator

@ShotaAk ShotaAk left a comment

Choose a reason for hiding this comment

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

動作OKです。細かいところにコメントしました

include/rt_usb_9axisimu_driver/rt_usb_9axisimu_driver.hpp Outdated Show resolved Hide resolved
include/rt_usb_9axisimu_driver/rt_usb_9axisimu_driver.hpp Outdated Show resolved Hide resolved
ShotaAk
ShotaAk approved these changes Aug 6, 2024
View reviewed changes
Copy link
Collaborator

@ShotaAk ShotaAk left a comment

Choose a reason for hiding this comment

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

LGTMです

@ShotaAk ShotaAk merged commit b22d060 into ros2-devel Aug 6, 2024
2 checks passed
@ShotaAk ShotaAk deleted the update_checkDataFormat branch August 6, 2024 05:58
This was referenced Aug 6, 2024
Closed
Closed
ShotaAk added a commit that referenced this pull request Aug 23, 2024
@YusukeKato @ShotaAk
2.1.0リリースのためにros2-develの更新内容をhumble-develへマージ (#60)
f1bdaf4 
* Add unit tests (#52)

* Use pointer for SerialPort instance instead of inheritance

* Add test using fakeit

* Install git comand in indsutrial_ci

* Refactoring

* Update CI

* Use AFTER_SETUP_TARGET_WORKSPACE

* driverインスタンスのメンバ変数のテストを追加

* バイナリデータが読み込まれたことを確認するテストを追加

* ASCIIデータが読み込まれたことを確認するテストを追加

* バイナリでもASCIIでもないデータが読み込まれたことを確認するテストを追加

* テストが通る状態に修正

---------

Co-authored-by: ShotaAk <[email protected]>

* checkDataFormat() and unit test updates (#54)

* Binaryデータ出力時は実機とテストの両方で動作確認完了、ASCIIデータには未対応、デバッグ用出力あり

* 実機とテストの両方でBinaryとASCIIの判定に成功、BinaryでもASCIIでもないデータには未対応

* タイムアウト機能を追加&hasCompletedFormatCheck()と関係する変数を削除

* 不正なデータをチェックするようにテストを修正

* readを成功させるためにwhileループにsleep処理を追加

* 必要がなかったため、while文のsleep処理を削除

* テスト用データの作成で重複している箇所を関数化

* RCLCPP_INFOをWARNに変更&不要なコメントを削除

* const autoをできる限り使用

* actions/checkoutのバージョンを3から4に更新

* 読み取ったデータをある程度貯めてからデータ形式を判定するように変更

* 貯めるデータを更新するように修正

* メンバ変数の名前を修正&256を定数化

* Add test for readSensorData() (#57)

* readSensorData()実行後のhasRefreshedImuData()の応答のテストを追加

* テストにコメントを追加

* 0.0を入力として与えたテストを追加

* ASCII形式のテストデータを作成する際、引数で値を渡せるように変更

* Binary形式でもテストデータを引数で渡せるように変更

* テストケースを使い回せるように変更

* readSensorData()実行時にセンサデータが正しく変換されたか検証するテストを追加

* デバッグ用の関数を削除

* short intをint16_tに変更

* int16を8bitずつに分ける関数をhighとlowそれぞれ用意

* 既存のテストに影響を与えないようにデータ作成の関数をオーバーロード

* set_data関数をprivateに変更

* 変換後の数値を直打ち

* data_format_を設定する関数を削除

* 不要な箇所を削除

* Fix to get the latest data (#58)

* 最新データ取得のテストを追加

* PR#50を参考に最新のデータを取得できるように変更

* 複数セットのデータ取得時に最新のデータをセットするように変更、テストは通るが挙動がおかしい

* 最新データの取得方法を修正

* 取得するデータが適切な長さであることを保証する

* 2.1.0リリースのためにCHANGELOG.rstとpackage.xmlを更新 (#59)

* CHANGELOGを更新

* 2.1.0

---------

Co-authored-by: ShotaAk <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ShotaAk ShotaAk ShotaAk approved these changes

Assignees

@YusukeKato YusukeKato

Labels
Type: Feature New Feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@YusukeKato @ShotaAk