Add API for getting moving status

[jonghee-son]

Added a new method, getMovingStatus, in the Dynamixel2Arduino class to retrieve the moving status item in control table of DYNAMIXEL actuator. This method includes logic to handle unsupported models.

[gemini-code-assist]

Hello @jonghee-son, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

Summary of Changes

This pull request adds a new method, getMovingStatus, to the Dynamixel2Arduino class. This method retrieves the moving status from the control table of a DYNAMIXEL actuator. The implementation includes specific handling for unsupported DYNAMIXEL models by returning an error code.

Highlights

• New API Method: Introduces getMovingStatus in Dynamixel2Arduino to fetch the moving status of DYNAMIXEL actuators.
• Unsupported Model Handling: Implements logic to identify and handle unsupported DYNAMIXEL models, setting an appropriate error code when encountered.

Changelog

• src/Dynamixel2Arduino.cpp
  • Added getMovingStatus method to retrieve moving status from the control table.
  • Implemented model number check to handle unsupported models and set the DXL_LIB_ERROR_NOT_SUPPORTED error code.
  • Calls readControlTableItem to read the MOVING_STATUS control table item for supported models.
• src/Dynamixel2Arduino.h
  • Added getMovingStatus method declaration to the Dynamixel2Arduino class.
  • Added MovingStatus enum to define the different moving status flags.
  • Added detailed documentation for the getMovingStatus method, including a code example.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

---

A servo's tale,
Status sought, no fail,
Motion's secrets known.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

The pull request introduces a new method getMovingStatus to the Dynamixel2Arduino class, which retrieves the moving status of a DYNAMIXEL actuator. The method includes logic to handle unsupported models by setting an appropriate error code. Overall, the implementation seems reasonable, but there are a few areas that could be improved for clarity and efficiency.

Summary of Findings

• Scalability of Unsupported Model Check: The current implementation of checking for unsupported models uses a long list of || conditions, which is not scalable or maintainable. Consider using a data structure like an array or hash table for better organization.
• Incorrect Example Code: The example code in the documentation for getMovingStatus uses an incorrect comparison (status & IN_POSITION == 1). This should be corrected to (status & IN_POSITION) == IN_POSITION or status & IN_POSITION to avoid confusion.
• Error Handling for Unregistered Models: The code attempts to set the model number if it's unregistered, but it doesn't handle the case where setModelNumber fails. This could lead to incorrect behavior if the model number cannot be determined.

Merge Readiness

The pull request introduces a useful feature, but there are some issues that should be addressed before merging. Specifically, the scalability of the unsupported model check and the incorrect example code should be fixed. The error handling for unregistered models should also be improved. I am unable to directly approve this pull request, and recommend that others review and approve this code before merging. At a minimum, the HIGH severity issue should be addressed before merging.

[gemini-code-assist]

The example code in the documentation uses status & IN_POSITION == 1. This is incorrect. Bitwise AND has lower precedence than equality. The code should use (status & IN_POSITION) == IN_POSITION or status & IN_POSITION. Please correct the example code to avoid confusion.

Suggested change

	* status = dxl.getMovingStatus(1);
	* if (status & IN_POSITION == 1) {
	* Serial.print("Arrived");
	* }
	* if (status & PROFILE_ONGOING == 1) {
	* Serial.print("Profile is in progress");
	* }
	* if (status & FOLLOWING_ERROR == 1) {
	* Serial.print("Not following desired position trajectory");
	* }
	* if (status & VELOCITY_PROFILE == 0xC0) {
	* Serial.print("Using trapezoidal profile");
	* }
	* else if (status & VELOCITY_PROFILE == 0x80) {
	* Serial.print("Using triangular profile");
	* }
	* else if (status & VELOCITY_PROFILE == 0x40) {
	* Serial.print("Using rectangular profile");
	* }
	* else {
	* Serial.print("Not using any profile (step)");
	* }
	* if (status & IN_POSITION) {
	* Serial.print("Arrived");
	* }

[gemini-code-assist]

This block checks for a long list of specific model numbers to determine if the MOVING_STATUS control table item is supported. This approach is not scalable and makes the code harder to maintain. Consider using a data structure (e.g., an array or a hash table) to store the unsupported model numbers and then check against that data structure. This would make the code more readable and easier to update in the future. Also, consider consolidating the two if statements into one, since they both perform the same action.

Could this be simplified by using a switch statement or a lookup table?

Suggested change

	if(model_num == AX12A || model_num == AX12W || model_num == AX18A || model_num == DX113 || model_num == DX116 || model_num == DX117 || model_num == RX10 || model_num == RX24F || model_num == RX28 || model_num == RX64 || model_num == EX106 || model_num == MX12W || model_num == MX28 || model_num == MX64 || model_num == MX106 || model_num == XL320)
	{
	setLastLibErrCode(DXL_LIB_ERROR_NOT_SUPPORTED);
	}
	else if(model_num == YM070_210_M001_RH || model_num == YM070_210_B001_RH || model_num == YM070_210_R051_RH || model_num == YM070_210_R099_RH || model_num == YM070_210_A051_RH || model_num == YM070_210_A099_RH || model_num == YM080_230_M001_RH || model_num == YM080_230_B001_RH || model_num == YM080_230_R051_RH || model_num == YM080_230_R099_RH || model_num == YM080_230_A051_RH || model_num == YM080_230_A099_RH)
	{
	setLastLibErrCode(DXL_LIB_ERROR_NOT_SUPPORTED);
	const uint16_t unsupported_models[] = {
	AX12A, AX12W, AX18A, DX113, DX116, DX117, RX10, RX24F, RX28, RX64, EX106, MX12W, MX28, MX64, MX106, XL320,
	YM070_210_M001_RH, YM070_210_B001_RH, YM070_210_R051_RH, YM070_210_R099_RH, YM070_210_A051_RH, YM070_210_A099_RH,
	YM080_230_M001_RH, YM080_230_B001_RH, YM080_230_R051_RH, YM080_230_R099_RH, YM080_230_A051_RH, YM080_230_A099_RH
	};
	bool is_supported = true;
	for (size_t i = 0; i < sizeof(unsupported_models) / sizeof(unsupported_models[0]); ++i) {
	if (model_num == unsupported_models[i]) {
	is_supported = false;
	break;
	}
	}
	if (!is_supported) {
	setLastLibErrCode(DXL_LIB_ERROR_NOT_SUPPORTED);
	} else {
	ret = (uint8_t)readControlTableItem(ControlTableItem::MOVING_STATUS, id);
	}

[gemini-code-assist]

This block attempts to set the model number if it's unregistered. However, if setModelNumber fails, model_num will remain UNREGISTERED_MODEL, and the subsequent checks will be performed with an invalid model number. It would be better to check the return value of setModelNumber and skip the unsupported model check if it fails. Also, the setModelNumber function already sets the last lib error code if it fails, so there is no need to set it again in the else block.

Suggested change

	if(model_num == UNREGISTERED_MODEL){
	if(setModelNumber(id, getModelNumber(id)) == true){
	model_num = getModelNumberFromTable(id);
	}
	}
	if(model_num == UNREGISTERED_MODEL){
	if(setModelNumber(id, getModelNumber(id)) == true){
	model_num = getModelNumberFromTable(id);
	} else {
	return 0; // Or some other appropriate error value
	}
	}