Skip to content

support non-actuator Dynamixel sensor devices (incl. HX Sensor Tip)#111

Open
jack0682 wants to merge 1 commit into
ROBOTIS-GIT:mainfrom
jack0682:feature/frontier-patches
Open

support non-actuator Dynamixel sensor devices (incl. HX Sensor Tip)#111
jack0682 wants to merge 1 commit into
ROBOTIS-GIT:mainfrom
jack0682:feature/frontier-patches

Conversation

@jack0682

Copy link
Copy Markdown

요약

Dynamixel 프로토콜을 사용하는 센서 전용 장치(HX Sensor Tip 등)를
액추에이터와 분리된 read 경로로 처리할 수 있도록 일반화합니다. ROBOTIS HX Sensor
Tip 모델 정의도 함께 포함합니다.

동기

ROBOTIS Dynamixel 생태계는 X-series 모터뿐 아니라 동일 프로토콜을 사용하는
다양한 센서 제품군(예: HX Sensor Tip)도 포함합니다. 그러나 현재
dynamixel_hardware_interface는 모든 장치를 동일한 SyncRead / FastSyncRead
그룹으로 묶기 때문에 다음과 같은 한계가 있습니다:

  1. 센서 1개의 통신 실패가 액추에이터 read 주기 전체를 지연/차단함
  2. 센서가 모터보다 낮은 주기로도 충분한 경우에도 동일 고주기로 강제 폴링됨
  3. 센서 측 에러 처리가 모터 제어 루프의 안정성에 영향을 미침

같은 버스 위에서 모터와 센서가 공존하는 구성은 ROBOTIS 표준 사용 패턴이므로,
이를 ros2_control 위에서 안정적으로 지원하기 위한 변경입니다.

주요 변경 사항

1. 센서 전용 read 경로 추가 (ReadSensorOnly)

  • 액추에이터 SyncRead 그룹과 분리된 read 호출을 통해 센서 데이터를 수집
  • 센서 측 통신 실패가 모터 read 그룹으로 전파되지 않도록 격리
  • 실패 시 silent fallback — 모터 제어 루프 영향 없음

2. tactile_read_divider 파라미터

  • 센서 read 주기를 모터 read 주기의 정수배로 분주 가능 (기본값 10)
  • 고주기 모터 제어와 저주기 센서 폴링을 동일 hardware interface에서 공존 가능
  • <hardware> 블록 파라미터로 URDF에서 설정 가능

3. HX Sensor Tip 모델 정의 추가

  • param/dxl_model/hx_sensor_tip.model 추가
  • ROBOTIS HX Sensor Tip의 control table 정의 포함
  • 기존 모델 정의 포맷과 동일한 스키마 사용

4. 기존 동작 보존

  • 새 파라미터 미설정 시 기존 동작과 완전히 동일
  • 모터만 사용하는 기존 사용자에게는 breaking change 없음

호환성

  • 기존 URDF / 설정 그대로 사용 시 동작 변화 없음
  • 새 기능은 <hardware> 파라미터로 opt-in
  • API / 플러그인 이름 변경 없음

테스트

  • OMY 7-DOF arm + HX Sensor Tip 조합으로 실기 검증
  • 센서 통신 실패 주입 테스트 시 모터 read 주기 영향 없음을 확인
  • 모터 전용 기존 구성에서도 회귀 없음 확인

관련 제품

  • ROBOTIS HX Sensor Tip (택타일 센서)
  • 향후 동일 프로토콜을 쓰는 추가 센서 제품군에도 동일 경로 재사용 가능

Signed-off-by: JaehongOh <jaehongoh1554@gmail.com>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a dedicated sensor-only read path to decouple tactile sensor data acquisition from the main actuator control loop, including support for the hx_sensor_tip model and configurable read pacing. It also improves error handling by throwing exceptions instead of exiting and attempts to resolve memory leaks in write operations. However, the review identified critical bugs: the new RAII-managed write buffers result in dangling pointers as the SDK does not copy the data, and the sensor read logic fails to apply unit scaling. Furthermore, the batching logic in ReadSensorOnly incorrectly assumes readTxRx can handle multiple device IDs, and the mandatory routing of GPIO sensors to the new path constitutes a breaking change that should be made optional.

Comment thread src/dynamixel/dynamixel.cpp
Comment thread src/dynamixel/dynamixel.cpp
Comment thread src/dynamixel/dynamixel.cpp
Comment thread src/dynamixel/dynamixel.cpp
Comment thread src/dynamixel_hardware_interface.cpp
@yun-goon yun-goon self-assigned this May 31, 2026
@yun-goon

Copy link
Copy Markdown
Member

I agree with the goal of keeping tactile / non-actuator sensors out of the actuator fast read group. However, this implementation still performs ReadSensorOnly() inside the ros2_control hardware read() loop every tactile_read_divider cycles.

On a shared U2D2 / DYNAMIXEL half-duplex bus, this means the sensor polling cycle gets an additional blocking readTxRx() transaction. Therefore, the actuator loop is not fully decoupled from sensor communication; the jitter is only moved to every Nth cycle.

This may be acceptable for low-rate, non-control-critical monitoring data, but it should be documented as such. For high-frequency or control-critical feedback, this can break the determinism of the read → update → write loop.

I think we need timing measurements, bounded timeout behavior, stale-data handling, and diagnostics / last-update information before merging this change.

@yun-goon

Copy link
Copy Markdown
Member

Also, please don’t resolve the Gemini PR review comments without replying to them first. If a comment is addressed, not applicable, or intentionally left unchanged, please leave a short explanation before marking it as resolved. This helps reviewers understand the decision and keeps the PR review history clear.

@jack0682

Copy link
Copy Markdown
Author

Thank you for the detailed review. I fully understand your concern, and I agree that the current implementation does not fully decouple tactile sensor communication from the actuator read loop.

As you pointed out, even though the tactile/non-actuator sensors are excluded from the actuator fast read group, calling ReadSensorOnly() inside the ros2_control hardware read() loop still introduces an additional blocking readTxRx() transaction every tactile_read_divider cycles on the shared U2D2 / DYNAMIXEL half-duplex bus. So the jitter is only reduced in frequency, not eliminated.

I also agree that this should be treated only as low-rate, non-control-critical monitoring data unless we add proper timing measurements, bounded timeout behavior, stale-data handling, diagnostics, and last-update visibility.

I’m sorry for resolving the Gemini PR review comments without replying first. That was not the right review practice. I will leave a short explanation on each comment before resolving it, whether the point was addressed, not applicable, or intentionally left unchanged.

I’ll revise the implementation and documentation accordingly, then request another review. Thank you again for catching this.

I agree with the goal of keeping tactile / non-actuator sensors out of the actuator fast read group. However, this implementation still performs ReadSensorOnly() inside the ros2_control hardware read() loop every tactile_read_divider cycles.

On a shared U2D2 / DYNAMIXEL half-duplex bus, this means the sensor polling cycle gets an additional blocking readTxRx() transaction. Therefore, the actuator loop is not fully decoupled from sensor communication; the jitter is only moved to every Nth cycle.

This may be acceptable for low-rate, non-control-critical monitoring data, but it should be documented as such. For high-frequency or control-critical feedback, this can break the determinism of the read → update → write loop.

I think we need timing measurements, bounded timeout behavior, stale-data handling, and diagnostics / last-update information before merging this change.

Also, please don’t resolve the Gemini PR review comments without replying to them first. If a comment is addressed, not applicable, or intentionally left unchanged, please leave a short explanation before marking it as resolved. This helps reviewers understand the decision and keeps the PR review history clear.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants