Improved actionsplayer

[randaz81]

No description provided.

[update-docs]

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would update the release notes by adding a file in doc/release/<target_branch>, based on your changes.

[gemini-code-assist]

Summary of Changes

Hello @randaz81, 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!

This pull request introduces a substantial architectural improvement to the yarpActionsPlayer module, enhancing its ability to manage and control robot actions across diverse hardware setups. By separating client connections from remapping logic, the module gains increased flexibility and maintainability. The changes also include better debugging capabilities through a new verbose logging option and more robust resource cleanup, ensuring the module operates efficiently and reliably. Furthermore, the controlBoardRemapper device's multi-client functionality is now thoroughly tested.

Highlights

• Refactored Configuration for yarpActionsPlayer: The yarpActionsPlayer module's configuration has been significantly refactored, moving from a single CONTROLLERS section to more granular CLIENTS and REMAPPERS sections. This change allows for greater flexibility in defining robot control boards and remapping joints across multiple clients.
• Introduced Verbose Logging Option: A new m_verbose flag and corresponding configuration option have been added. When enabled, this flag allows for more detailed debug messages to be printed during action execution, aiding in debugging and monitoring.
• Improved Resource Management and Cleanup: The scriptModule destructor now includes explicit logic to stop and delete worker threads (m_wthread, m_bthread) and to clear all dynamically allocated PolyDriver instances for clients and remappers, as well as action storage, preventing potential memory leaks.
• Modular robotDriver Integration: The robotDriver class has been updated to accept a yarp::dev::PolyDriver* directly in its configure method. This makes the driver more modular and decouples its configuration from specific property parsing, aligning it with the new remapping architecture.
• Expanded controlBoardRemapper Test Coverage: A new test case has been added to controlBoardRemapper_t3_test.cpp to validate the controlboardremapper device's functionality when attached to two controlBoard_nwc_yarp devices, ensuring its multi-client remapping capabilities are robust.

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 by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

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 pull request 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 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. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

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.

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. ↩

[randaz81]

/gemini review

[randaz81]

/gemini review

[randaz81]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces significant improvements to the yarpActionsPlayer by refactoring its internal architecture to use PolyDriver for managing robot clients and remappers. This change enhances modularity and flexibility in configuring robot control. The configuration file format has been updated to reflect this new structure, and corresponding changes have been made in main.cpp to parse these new sections. Additionally, verbosity control has been added to the ControlThread, and the destructor logic has been improved for better resource management. New test cases have also been added to cover the updated remapping functionality.

However, during the review, several critical issues were identified in main.cpp related to the initialization and usage of PolyDriver and robotDriver instances within the loadConfiguration method. These issues involve dereferencing null pointers, which would lead to crashes at runtime. Please address these issues to ensure the stability and correctness of the new architecture.

[randaz81]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces significant improvements to the yarpActionsPlayer by refactoring the configuration to support multiple control board clients and remappers, making the player more flexible and powerful. The resource management in the destructor has also been made more robust by correctly stopping threads and clearing all allocated resources. I've added a few suggestions to improve maintainability, code clarity, and correctness.

[gemini-code-assist]

The member variable m_verbose is public. It's good practice to make member variables private to enforce encapsulation and control access. Please consider moving it to the private section and adding a public setter method, e.g., void setVerbose(bool verbose), to modify its value from scriptModule.

[gemini-code-assist]

The logic for stopping and deleting threads is duplicated for m_wthread and m_bthread. This could be extracted into a helper function to avoid repetition and improve readability. For example:

template<typename T>
void stopAndDeleteThread(T*& thread)
{
    if (thread)
    {
        thread->stop();
        yarp::os::Time::delay(0.100);
        delete thread;
        thread = nullptr;
    }
}

Then you can call stopAndDeleteThread(m_wthread); and stopAndDeleteThread(m_bthread);.

[gemini-code-assist]

Using m_robotClients[client_name] to check for existence has the side effect of inserting a new element if client_name is not found. While it works here because the default-constructed pointer is nullptr, it's cleaner and safer to use m_robotClients.count(client_name) > 0. This is also consistent with how you check for duplicates in m_robotRemappers and m_robotControllers later in the code.

Suggested change

	if (m_robotClients[client_name] != nullptr)
	if (m_robotClients.count(client_name) > 0)

[gemini-code-assist]

The error messages error in clients_name and error in axes_names are not very descriptive. It would be more helpful for debugging to provide more context, such as the name of the remapper being processed. For example:

• yError() << "Invalid 'clients_name' for remapper '" << remapper_name << "': it is null or empty.";
• yError() << "Invalid 'axes_names' for remapper '" << remapper_name << "': it is empty.";