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

Add quadrant photodiode sensor (without noises) #81

Merged
merged 33 commits into from
Jan 10, 2024

Conversation

TomokiMochizuki
Copy link
Member

@TomokiMochizuki TomokiMochizuki commented Jan 4, 2024

概要

Add quadrant photodiode sensor (without noises)

Issue

  • 関連する issue

詳細

Add quadrant photodiode sensor

  • このPRではノイズ特性については模擬していない.
    • ノイズ特性の模擬は別PRで行う予定.

検証結果

[注意] 以下のシミュレーション結果は意図的にレーザー径を参照値に一致させたときの結果を示している

  • 理由:
    • レーザー径が変化すると理論的に位置決定精度が悪化してしまうため
    • 位置決定精度の悪化に対応するアルゴリズムは別PRで実装予定

実際の変位と四分割センサを用いて観測した変位
image

四分割センサの出力値
image

実際の変位と位置決定値との差
image

影響範囲

XX系の動作がガラッと変わる,とか.

補足

何かあれば

注意

  • 関連する Projects が存在する場合,それの紐付けを行うこと
  • Assignees を設定すること
  • 可能ならば Reviewers を設定すること
  • 可能ならば priority ラベルを付けること

@TomokiMochizuki TomokiMochizuki removed the request for review from 200km January 4, 2024 10:04
@TomokiMochizuki TomokiMochizuki self-assigned this Jan 4, 2024
@TomokiMochizuki TomokiMochizuki added the priority::high priorityg high label Jan 4, 2024
s2e-ff/src/components/aocs/laser_emitter.hpp Outdated Show resolved Hide resolved
s2e-ff/src/components/aocs/quadrant_photodiode_sensor.cpp Outdated Show resolved Hide resolved
s2e-ff/src/components/aocs/quadrant_photodiode_sensor.hpp Outdated Show resolved Hide resolved
s2e-ff/src/components/aocs/quadrant_photodiode_sensor.hpp Outdated Show resolved Hide resolved
s2e-ff/src/components/aocs/quadrant_photodiode_sensor.cpp Outdated Show resolved Hide resolved
s2e-ff/src/components/aocs/quadrant_photodiode_sensor.cpp Outdated Show resolved Hide resolved
s2e-ff/src/components/aocs/quadrant_photodiode_sensor.cpp Outdated Show resolved Hide resolved
s2e-ff/src/components/aocs/quadrant_photodiode_sensor.cpp Outdated Show resolved Hide resolved
s2e-ff/src/components/aocs/quadrant_photodiode_sensor.cpp Outdated Show resolved Hide resolved
* @brief Type of the quadrant photodiode sensor output value
*/
typedef enum {
VOLTAGE_HORIZONTAL = 0, //!< Value corresponding to the horizontal displacement
Copy link
Member

Choose a reason for hiding this comment

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

yAxisDirectionに修正されていますが、スタイルガイドに準拠するなら、kYAxisDirectionが正しいと思います。

@200km
Copy link
Member

200km commented Jan 5, 2024

全体を通してのコメントです。今回実装されたQPDセンサは、「QPDという光学素子 + 位置を出すための計算」ということで、最終出力は「センサ面内のレーザーの当たった位置情報」になっています。この場合、QPDセンサと素子名をダイレクトに使った命名は違和感があります。例えば、サンセンサを「フォトダイオードセンサ」、STTをCCDセンサと呼んでいるような感じです。

そしてこの命名の違和感により、コード内の細かな部分の違和感につながっているような気がします。

この種のセンサの呼び方の共通認識があるわけではないので難しいところですが、InPlanePositionSensorWithQpdのような命名が適切な気がします。もっと短くて適切な命名があればそちらでも良いとは思います。

@TomokiMochizuki
Copy link
Member Author

全体を通してのコメントです。今回実装されたQPDセンサは、「QPDという光学素子 + 位置を出すための計算」ということで、最終出力は「センサ面内のレーザーの当たった位置情報」になっています。この場合、QPDセンサと素子名をダイレクトに使った命名は違和感があります。例えば、サンセンサを「フォトダイオードセンサ」、STTをCCDセンサと呼んでいるような感じです。

そしてこの命名の違和感により、コード内の細かな部分の違和感につながっているような気がします。

この種のセンサの呼び方の共通認識があるわけではないので難しいところですが、InPlanePositionSensorWithQpdのような命名が適切な気がします。もっと短くて適切な命名があればそちらでも良いとは思います。

承知しました.
後ほど少し考えさせていただきます.

@TomokiMochizuki
Copy link
Member Author

TomokiMochizuki commented Jan 5, 2024

全体を通してのコメントです。今回実装されたQPDセンサは、「QPDという光学素子 + 位置を出すための計算」ということで、最終出力は「センサ面内のレーザーの当たった位置情報」になっています。この場合、QPDセンサと素子名をダイレクトに使った命名は違和感があります。例えば、サンセンサを「フォトダイオードセンサ」、STTをCCDセンサと呼んでいるような感じです。

そしてこの命名の違和感により、コード内の細かな部分の違和感につながっているような気がします。

この種のセンサの呼び方の共通認識があるわけではないので難しいところですが、InPlanePositionSensorWithQpdのような命名が適切な気がします。もっと短くて適切な命名があればそちらでも良いとは思います。

こちらですが,QpdSensorPositioningSystem, QpdSensorSystemとかはいかがでしょう?

@200km
Copy link
Member

200km commented Jan 5, 2024

提案ありがとうございます。QpdPositioningSensorとかQpdPositioningSystemがしっくりくる気がします。あとは、PROBA-3の例に倣って、QpdLlsとかもありな気もします。

@TomokiMochizuki
Copy link
Member Author

提案ありがとうございます。QpdPositioningSensorとかQpdPositioningSystemがしっくりくる気がします。あとは、PROBA-3の例に倣って、QpdLlsとかもありな気もします。

位置の観測はセンサというよりかはシステムな気がするのでQpdPositioningSystemとしました.

@200km
Copy link
Member

200km commented Jan 5, 2024

sun sensorやstar sensorなどと同じような感じなので、systemとまでいうほどではない気がしますけどね。
sun sensorもフォトダイオード複数使って太陽方向決めたりするので、ほぼ同じような構成、仕組み、規模間な気がしています。

@200km
Copy link
Member

200km commented Jan 5, 2024

Systemは、複数のQpdPositioningSensorやLaser距離計を使って、位置・姿勢6自由度全て推定する場合など、もう少し規模が大きい場合に使った方がしっくりくる気がしますね。

@TomokiMochizuki
Copy link
Member Author

Systemは、複数のQpdPositioningSensorやLaser距離計を使って、位置・姿勢6自由度全て推定する場合など、もう少し規模が大きい場合に使った方がしっくりくる気がしますね。

では、QpdPositioningSensorにします

s2e-ff/src/components/aocs/qpd_positioning_sensor.cpp Outdated Show resolved Hide resolved

// It is required to have a position calculation converter based on the output values of the quadrant sensors.
// The following file contains the arrays required to determine the position displacements.
// reference_ratio_y_axis_list : List of `qpd_sensor_output_y_axis_V / qpd_sensor_output_sum_V` at each point on the y-axis.
Copy link
Member

Choose a reason for hiding this comment

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

[NITS] この内容はCSVファイルの方に書かれているべきではないでしょうか?

Copy link
Member Author

Choose a reason for hiding this comment

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

s2e-coreを見ても基本的に,CSVファイルはTableとしてしか使用していないので,iniファイルで詳細な説明はしたほうがいいと判断しました.

Copy link
Member

Choose a reason for hiding this comment

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

逆にiniファイルに書いているものもないとは思います。他の人にとってサンプルとなり得るcsvファイル自体に書かれていた方がユーザー目線では使いやすいかなと思います。
他のs2e-coreに揃えるということなら、iniファイルよりもクラス説明自体に書くことが自然ということになります。

@@ -0,0 +1,802 @@
displacement[m],reference_ratio_y_axis_list,reference_ratio_z_axis_list
Copy link
Member

Choose a reason for hiding this comment

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

[NITS] iniの方に書かれている説明を読んでも、referenceという命名が適切な理由がやはりよくわかりませんでした。ただのratiovoltage_ratioではダメなんですかね?

Copy link
Member Author

Choose a reason for hiding this comment

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

位置変動に対するセンサの出力値は衛星間距離が異なると変化してしまうので、衛星間距離がどういう値の時のvoltage_ratioを使用しているのかを表現する必要があり、こういう表記になっていますが、referenceは不適切かもしれません。

Copy link
Member

Choose a reason for hiding this comment

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

衛星間距離自体も測れるわけではないので、sumの電圧で情報をとっているんですよね?

とりあえずreferenceはどういう意味かわからないので他のものが良いと思います。最終的には、documentを書いてそこで丁寧に説明してください。

Copy link
Member Author

@TomokiMochizuki TomokiMochizuki Jan 10, 2024

Choose a reason for hiding this comment

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

四分割センサでは衛星間距離は計算できないものとしています。

このテーブルにあるdisplacement[m]衛星間距離ということではなく、y軸方向, z軸方向の変位の真値を表していて、その真値の時に各voltage_ratioがどのような値をとるのかを示すために置いています。

// the y-z plane in the component coordinate system.
// The x-axis is the direction normal to the plane of sensitivity of the quadrant photodiode sensor.
// The output values of this quadrant photodiode sensor is listed as follows:
// qpd_sensor_output_y_axis_V : This value changes corresponding to the displacement in the y-axis direction.
Copy link
Member

Choose a reason for hiding this comment

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

[NITS] iniファイルに出力の情報が書かれていてもあまり意味がない気がしています。書くべきは、Get関数などが定義されているクラスのhppファイルではないでしょうか?

Copy link
Member Author

Choose a reason for hiding this comment

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

hppファイルにも記載しています

double qpd_sensor_output_y_axis_V_; //!< Quadrant photodiode sensor output value corresponding to the y-axis direction [V]
double qpd_sensor_output_z_axis_V_; //!< Quadrant photodiode sensor output value corresponding to the y-axis direction [V]
double qpd_sensor_output_sum_V_; //!< Quadrant photodiode sensor output value corresponding to the sum of the light intensity [V]

Copy link
Member Author

@TomokiMochizuki TomokiMochizuki Jan 8, 2024

Choose a reason for hiding this comment

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

ここで記載しているのは、iniファイル内でvoltage_ratioの説明をするときにあったほうが便利だからです.

// It is required to have a position calculation converter based on the output values of the quadrant sensors.
// The following file contains the arrays required to determine the position displacements.
// reference_ratio_y_axis_list : List of `qpd_sensor_output_y_axis_V / qpd_sensor_output_sum_V` at each point on the y-axis.
// reference_ratio_z_axis_list : List of `qpd_sensor_output_z_axis_V / qpd_sensor_output_sum_V` at each point on the z-axis.

Copy link
Member

Choose a reason for hiding this comment

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

では、別で書いているように全てheaderファイルに書くようにしましょう。複数の部分に書かれていたら、今後の編集でどちらか一つだけ書き換えられて、もう一方が書き換えられないということが起きやすいと思います。
どうしてもiniに情報を書きたいなら、headerファイルを見てくださいとか参照を使う方が良いと思います。

// reference_ratio_z_axis_list : List of `qpd_sensor_output_z_axis_V / qpd_sensor_output_sum_V` at each point on the z-axis.
qpd_sensor_file_directory = ../../data/initialize_files/components/qpd_sensor_csv_files/

qpd_sensor_radius_m = 3.9e-3
Copy link
Member

Choose a reason for hiding this comment

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

[NITS]iniに書くべきコメントは、どちらかというとここら辺のパラメータ設定の意味だと思います。

s2e-ff/src/components/aocs/qpd_positioning_sensor.cpp Outdated Show resolved Hide resolved
s2e-ff/src/components/aocs/qpd_positioning_sensor.cpp Outdated Show resolved Hide resolved
s2e-ff/src/components/aocs/qpd_positioning_sensor.cpp Outdated Show resolved Hide resolved
Copy link
Member

@200km 200km left a comment

Choose a reason for hiding this comment

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

残りはコメントやdocument関連のコメントだけなので、approveします。

@TomokiMochizuki TomokiMochizuki merged commit a1ca8b5 into develop Jan 10, 2024
8 checks passed
@TomokiMochizuki TomokiMochizuki deleted the feature/add_qpd_sensor branch January 10, 2024 07:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority::high priorityg high
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants