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

IEP-374: GCOV Reports view and creation #817

Open
wants to merge 26 commits into
base: master
Choose a base branch
from
Open

IEP-374: GCOV Reports view and creation #817

wants to merge 26 commits into from

Conversation

alirana01
Copy link
Collaborator

@alirana01 alirana01 commented Sep 6, 2023

Description

New feature to add the ability to create and view the gcov reports inside the eclipse

Fixes # (IEP-374)

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

How has this been tested?

Creating gcov reports using the gcov example

Test Configuration:

  • ESP-IDF Version: any
  • OS (Windows,Linux and macOS): all

Checklist

  • PR Self Reviewed
  • Applied Code formatting
  • Added Documentation
  • Verified on all platforms - Windows,Linux and macOS

Summary by CodeRabbit

Summary by CodeRabbit

  • New Features
    • Added support for GCOV code coverage and dump generation in the IDF Eclipse plugin. Users can now generate and view GCOV reports directly from the IDE.
    • Introduced a new GcovFileView in the UI to display gcno/gcda files information in a table format.
    • Added two new commands, "GCOV Instant" and "GCOV Hard-coded", for dumping GCOV data.
  • Refactor
    • Renamed handleWebSocketClientInstall() to handlePythonDependenciesInstall(), reflecting the change in dependency installation process.
  • Chore
    • Updated Python dependencies installation to include gcovr package along with websocket-client.
  • Documentation
    • Updated README.md with instructions on how to use the new GCOV features.

@coderabbitai
Copy link

coderabbitai bot commented Sep 6, 2023

Walkthrough

This update introduces significant enhancements to the ESP-IDF plugin for Eclipse, focusing on Gcov integration for code coverage analysis. It adds utility classes and methods for managing Gcov files, commands for instant Gcov data dumping, and a new view for displaying Gcov reports. Additionally, the update expands Python dependencies installation to include gcovr, streamlines the handling of launch configurations for OpenOCD, and revises the documentation to cover the new Gcov features. Overall, these changes aim to improve the development and debugging experience within the Eclipse IDE.

Changes

File Path Change Summary
bundles/com.espressif.idf.core/META-INF/MANIFEST.MF
bundles/com.espressif.idf.ui/META-INF/MANIFEST.MF
Updated Require-Bundle section; added and removed several bundles.
bundles/com.espressif.idf.core/src/.../GcovUtility.java
bundles/com.espressif.idf.ui/src/.../gcov/GcovFileView.java
bundles/com.espressif.idf.ui/src/.../gcov/GcovFileViewCommandHandler.java
bundles/com.espressif.idf.ui/src/.../gcov/Messages.java
Added new classes for Gcov file management and UI views.
bundles/com.espressif.idf.debug.gdbjtag.openocd/plugin.xml Added new commands for Gcov data dumping.
bundles/com.espressif.idf.debug.gdbjtag.openocd/src/.../common/LaunchConfigHandler.java
bundles/com.espressif.idf.debug.gdbjtag.openocd/src/.../gcov/GcovDumpHandler.java
Added new classes for launch configuration handling and Gcov dumping.
bundles/com.espressif.idf.serial.monitor/src/.../core/IDFMonitor.java
bundles/com.espressif.idf.ui/src/.../update/InstallToolsHandler.java
Updated method calls for Python dependencies installation.
bundles/com.espressif.idf.ui/plugin.xml Added commands and a menu contribution for Gcov files.
bundles/com.espressif.idf.ui/src/.../LogMessagesThread.java Changed package name; no functionality changes.
bundles/com.espressif.idf.ui/src/.../tools/ToolsInstallationHandler.java
bundles/com.espressif.idf.ui/src/.../update/InstallToolsHandler.java
Renamed method to handlePipDependenciesInstall(); expanded scope to include gcovr.
bundles/com.espressif.idf.ui/src/.../tools/wizard/pages/... Updated import statement for LogMessagesThread.
README.md
README_CN.md
Updated documentation to include GCOV Code Coverage and Dump Generation section.

🐰✨
In the land of code and wire,
A rabbit hopped, with heart afire.
"Gcov's in town," it did proclaim,
With each file change, a new game.
Coverage maps, in Eclipse they bloom,
Lighting up the path, dispelling gloom.
🌟📈🐾

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit-tests for this file.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit tests for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository from git and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit tests.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • The JSON schema for the configuration file is available here.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

CodeRabbit Discord Community

Join our Discord Community to get help, request features, and share feedback.

Copy link
Collaborator Author

@alirana01 alirana01 left a comment

Choose a reason for hiding this comment

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

Self Review will do some changes in coming commits

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 10

Commits Files that changed from the base of the PR and between 9d6c775 and c2eff88 commits.
Files selected for processing (18)
  • bundles/com.espressif.idf.core/META-INF/MANIFEST.MF (1 hunks)
  • bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/GcovUtility.java (1 hunks)
  • bundles/com.espressif.idf.debug.gdbjtag.openocd/plugin.xml (1 hunks)
  • bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/common/LaunchConfigHandler.java (1 hunks)
  • bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/dsf/Launch.java (2 hunks)
  • bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/gcov/GcovDumpHandler.java (1 hunks)
  • bundles/com.espressif.idf.serial.monitor/src/com/espressif/idf/serial/monitor/core/IDFMonitor.java (1 hunks)
  • bundles/com.espressif.idf.ui/META-INF/MANIFEST.MF (1 hunks)
  • bundles/com.espressif.idf.ui/plugin.xml (4 hunks)
  • bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/LogMessagesThread.java (1 hunks)
  • bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/gcov/GcovFileView.java (1 hunks)
  • bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/gcov/GcovFileViewCommandHandler.java (1 hunks)
  • bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsInstallationHandler.java (3 hunks)
  • bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/wizard/pages/InstallEspIdfPage.java (1 hunks)
  • bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/wizard/pages/ManageToolsInstallationWizardPage.java (1 hunks)
  • bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/InstallToolsHandler.java (3 hunks)
  • bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/Messages.java (1 hunks)
  • bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/messages.properties (1 hunks)
Files skipped from review due to trivial changes (9)
  • bundles/com.espressif.idf.debug.gdbjtag.openocd/plugin.xml
  • bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/gcov/GcovDumpHandler.java
  • bundles/com.espressif.idf.ui/plugin.xml
  • bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/LogMessagesThread.java
  • bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/gcov/GcovFileView.java
  • bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/wizard/pages/ManageToolsInstallationWizardPage.java
  • bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/InstallToolsHandler.java
  • bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/Messages.java
  • bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/messages.properties
Additional comments (Suppressed): 10
bundles/com.espressif.idf.ui/META-INF/MANIFEST.MF (2)
  • 41-45: The new dependency org.eclipse.core.variables;bundle-version="3.5.100" has been added. Ensure that this version of the package is available in the environment where this plugin will be deployed.

  • 49-54: The new package com.espressif.idf.ui.gcov is being exported. This aligns with the PR summary which mentioned the addition of a new package for handling Gcov files and reports.

bundles/com.espressif.idf.core/META-INF/MANIFEST.MF (2)
  • 30-46: The new dependencies added are all related to the Linux Tools project, which includes tools for Linux development in general and some specific to embedded systems. Given that this PR is about adding Gcov support, these additions make sense. However, please verify if all these dependencies are necessary for your feature. Unnecessary dependencies can lead to a bloated application and potential version conflicts.

  • 47-47: Ensure that the JavaSE-11 execution environment is still appropriate with the addition of these new dependencies. Some may require a newer version of Java.

bundles/com.espressif.idf.serial.monitor/src/com/espressif/idf/serial/monitor/core/IDFMonitor.java (1)
  • 192-193: Ensure that the new method handlePythonDependenciesInstall() in InstallToolsHandler class correctly installs both websocket-client and gcovr packages as mentioned in the PR summary.
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/GcovUtility.java (1)
  • 1-4: Ensure that the copyright notice is up-to-date and correctly formatted.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/wizard/pages/InstallEspIdfPage.java (1)
  • 38-38: The import statement for LogMessagesThread has been moved. Ensure that this change does not affect the order of execution or dependencies in the code.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsInstallationHandler.java (3)
  • 570-576: The method handlePipDependenciesInstall() is called here. Ensure that all calls to the old method handleWebSocketClientInstall() throughout the codebase have been updated to this new method.

  • 870-873: The method name has been changed from handleWebSocketClientInstall() to handlePipDependenciesInstall(). This change reflects the fact that the method now handles the installation of more than just the WebSocket client.

  • 885-887: The gcovr package has been added to the list of packages to install. This package is necessary for generating Gcov reports.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 2

Commits Files that changed from the base of the PR and between c2eff88 and 75eb613 commits.
Files selected for processing (1)
  • bundles/com.espressif.idf.core/META-INF/MANIFEST.MF (1 hunks)
Additional comments (Suppressed): 1
bundles/com.espressif.idf.core/META-INF/MANIFEST.MF (1)
  • 25-47: The new dependencies added to the manifest file are related to Linux tools, profiling, Docker, and Gcov. Ensure that these new dependencies do not introduce any compatibility issues with the existing codebase or the target runtime environment. Also, verify if all these dependencies are necessary for the project's requirements.

bundles/com.espressif.idf.core/META-INF/MANIFEST.MF Outdated Show resolved Hide resolved
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 1

Commits Files that changed from the base of the PR and between 75eb613 and bbd6b0a commits.
Files selected for processing (1)
  • bundles/com.espressif.idf.core/META-INF/MANIFEST.MF (1 hunks)
Additional comments (Suppressed): 1
bundles/com.espressif.idf.core/META-INF/MANIFEST.MF (1)
  • 25-46: The new dependencies added to the manifest file are related to Linux tools, profiling, Docker, and Gcov. Ensure that these new dependencies do not introduce any breaking changes or conflicts with existing dependencies. Also, verify if all these new dependencies are necessary for the functionality of the bundle.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 3

Commits Files that changed from the base of the PR and between bbd6b0a and 530ad63 commits.
Files selected for processing (2)
  • bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/dsf/Launch.java (3 hunks)
  • bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/gcov/GcovFileViewCommandHandler.java (1 hunks)
Additional comments (Suppressed): 1
bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/dsf/Launch.java (1)
  • 38-43: The import statement for LaunchConfigurationWorkingCopy has been removed, which is a good change if it's not being used in the code. Removing unused imports helps to keep the code clean and maintainable.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 0

Commits Files that changed from the base of the PR and between 530ad63 and 0e4a3ca commits.
Files selected for processing (3)
  • bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/gcov/GcovFileView.java (1 hunks)
  • bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/gcov/Messages.java (1 hunks)
  • bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/gcov/messages.properties (1 hunks)
Files skipped from review due to trivial changes (3)
  • bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/gcov/GcovFileView.java
  • bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/gcov/Messages.java
  • bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/gcov/messages.properties

@alirana01 alirana01 self-assigned this Sep 7, 2023
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 0

Commits Files that changed from the base of the PR and between 0e4a3ca and f830dcd commits.
Files selected for processing (1)
  • tests/com.espressif.idf.ui.test/META-INF/MANIFEST.MF (1 hunks)
Additional comments (Suppressed): 2
tests/com.espressif.idf.ui.test/META-INF/MANIFEST.MF (2)
  • 13-34: Ensure that all the newly added dependencies are available in the build environment and they are compatible with each other. Also, verify if these new dependencies introduce any breaking changes or conflicts with existing code.

  • 35-37: The Import-Package directive is used to specify the packages that this bundle requires from other bundles. Ensure that the imported packages are available and compatible with the current version of the project.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 1

Commits Files that changed from the base of the PR and between f830dcd and 1f4ffea commits.
Files selected for processing (1)
  • bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/gcov/GcovFileViewCommandHandler.java (1 hunks)

Copy link
Collaborator

@sigmaaa sigmaaa left a comment

Choose a reason for hiding this comment

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

Hi @alirana01, thanks for the PR, mostly looks good to me. I wrote down some notes. I would also consider removing too deep nesting in some places and using lambda expressions instead of anonymous classes where it is possible to increase readability, but that's not that critical and up to you.
I will also make a functionality review a little bit later.

Comment on lines +102 to +113
ILaunchConfiguration launchConfig = finalActiveLaunch.getLaunchConfiguration();
Display.getDefault().asyncExec(() -> {
String projectName;
try
{
projectName = launchConfig.getAttribute(
ICDTLaunchConfigurationConstants.ATTR_PROJECT_NAME, StringUtil.EMPTY);

if (!projectName.isEmpty())
{
IProject project = ResourcesPlugin.getWorkspace().getRoot()
.getProject(projectName);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can get the project directly from the launch configuration's mapped resources. Something like this: configuration.getMappedResources()[0].getProject(); or something like this if you want to avoid null checks:

		IResource[] resources = null;
		try
		{
			resources = config.getMappedResources();
		}
		catch (CoreException e)
		{
			Logger.log(e);
		}
		Optional<IProject> projectOptional = resources == null ? Optional.empty()
				: Stream.of(resources).filter(Objects::nonNull)
						.filter(resource -> resource.getType() == IResource.PROJECT).map(IResource::getProject)
						.findFirst();
	}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am not clear on this we can use that approach but this should be fine too I think since the attribute ID comes from internal static constant

Comment on lines +126 to +127
GcovFileView gcovFileView = (GcovFileView) page
.showView(GcovFileView.ID);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure if that's okay to use the class from the UI package here, since it can cause cyclic dependency later. Isn't it better to move this handler to the com.espressif.idf.ui.handlers package?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There are some launch class dependencies there that are required and I am not sure if that can be good to add this package as a dependency to ui project

Comment on lines 96 to 98
IFile file = (IFile) item.getData();

if (item != null && file != null)
Copy link
Collaborator

Choose a reason for hiding this comment

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

possible NPE here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

Comment on lines +166 to +223
if (getSelectedProject() == null)
{
setSelectedProject(GcovUtility.getSelectedProject());
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the Utility class really necessary for this? I believe we can retrieve a mapped project from the active launch configuration at this moment, can't we?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I did this because we may have multiple projects and user can right click on the project and launch this view

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 0

Commits Files that changed from the base of the PR and between 1f4ffea and 9048367 commits.
Files selected for processing (1)
  • releng/com.espressif.idf.target/com.espressif.idf.target.target (2 hunks)
Files skipped from review due to trivial changes (1)
  • releng/com.espressif.idf.target/com.espressif.idf.target.target

Comment on lines 127 to 130
<unit id="org.eclipse.linuxtools.dataviewers.feature.feature.group" version="8.11.0.202306052033"/>
<unit id="org.eclipse.linuxtools.gprof.feature.feature.group" version="8.11.0.202306052033"/>
<unit id="org.eclipse.linuxtools.profiling.feature.group" version="8.11.0.202306052033"/>
<unit id="org.eclipse.linuxtools.profiling.remote.feature.group" version="8.11.0.202306052033"/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi @alirana01 Do we need all the features from the linuxtools? I thought we only need org.eclipse.linuxtools.gcov-feature? Also avoid mentioning a specific version so that it will continue to work with the latest versions when it's available.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 0

Commits Files that changed from the base of the PR and between 9048367 and ea93fc5 commits.
Files selected for processing (1)
  • releng/com.espressif.idf.target/com.espressif.idf.target.target (2 hunks)
Files skipped from review due to trivial changes (1)
  • releng/com.espressif.idf.target/com.espressif.idf.target.target

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 0

Commits Files that changed from the base of the PR and between ea93fc5 and c3f39c0 commits.
Files selected for processing (1)
  • bundles/com.espressif.idf.core/META-INF/MANIFEST.MF (1 hunks)
Files skipped from review due to trivial changes (1)
  • bundles/com.espressif.idf.core/META-INF/MANIFEST.MF

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 2

Commits Files that changed from the base of the PR and between c3f39c0 and b7fd36d commits.
Files selected for processing (1)
  • bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/gcov/GcovFileView.java (1 hunks)
Additional comments (Suppressed): 5
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/gcov/GcovFileView.java (5)
  • 73-128: The createPartControl method is responsible for creating the UI components of the view. It creates a table with multiple columns and sets up listeners for various events such as double-clicking on a row. The method also adds a "Refresh" action to the toolbar, which calls the refreshList method when clicked. This method seems well-structured and doesn't appear to have any significant issues.

  • 130-141: This selectionChanged method updates the selected project based on the current selection in the workbench. It checks if the selection is an instance of IStructuredSelection and if the first element of the selection is an instance of IProject. If both conditions are met, it sets the selected project. This method seems to be implemented correctly.

  • 269-273: The setFocus method sets the focus to the table when the view is focused. This method seems to be implemented correctly.

  • 275-280: The dispose method removes the selection listener from the page and calls the superclass's dispose method. This method seems to be implemented correctly.

  • 282-290: The getSelectedProject and setSelectedProject methods are simple getter and setter methods for the selectedProject field. These methods seem to be implemented correctly.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 0

Commits Files that changed from the base of the PR and between b7fd36d and 931da92 commits.
Files selected for processing (3)
  • bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/gcov/GcovFileView.java (1 hunks)
  • bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/gcov/Messages.java (1 hunks)
  • bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/gcov/messages.properties (1 hunks)
Files skipped from review due to trivial changes (3)
  • bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/gcov/GcovFileView.java
  • bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/gcov/Messages.java
  • bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/gcov/messages.properties

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 10

Commits Files that changed from the base of the PR and between 6ff9814 and e9289a8.
Files selected for processing (20)
  • bundles/com.espressif.idf.core/META-INF/MANIFEST.MF (1 hunks)
  • bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/GcovUtility.java (1 hunks)
  • bundles/com.espressif.idf.debug.gdbjtag.openocd/plugin.xml (1 hunks)
  • bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/dsf/Launch.java (3 hunks)
  • bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/gcov/GcovDumpHandler.java (1 hunks)
  • bundles/com.espressif.idf.serial.monitor/src/com/espressif/idf/serial/monitor/core/IDFMonitor.java (1 hunks)
  • bundles/com.espressif.idf.ui/META-INF/MANIFEST.MF (1 hunks)
  • bundles/com.espressif.idf.ui/plugin.xml (4 hunks)
  • bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/LogMessagesThread.java (1 hunks)
  • bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/gcov/GcovFileView.java (1 hunks)
  • bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/gcov/GcovFileViewCommandHandler.java (1 hunks)
  • bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/gcov/Messages.java (1 hunks)
  • bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/gcov/messages.properties (1 hunks)
  • bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsInstallationHandler.java (3 hunks)
  • bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/wizard/pages/InstallEspIdfPage.java (1 hunks)
  • bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/wizard/pages/ManageToolsInstallationWizardPage.java (1 hunks)
  • bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/InstallToolsHandler.java (3 hunks)
  • bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/Messages.java (1 hunks)
  • bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/messages.properties (1 hunks)
  • releng/com.espressif.idf.target/com.espressif.idf.target.target (2 hunks)
Files skipped from review due to trivial changes (10)
  • bundles/com.espressif.idf.core/META-INF/MANIFEST.MF
  • bundles/com.espressif.idf.debug.gdbjtag.openocd/plugin.xml
  • bundles/com.espressif.idf.ui/plugin.xml
  • bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/LogMessagesThread.java
  • bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/gcov/GcovFileViewCommandHandler.java
  • bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/gcov/Messages.java
  • bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/gcov/messages.properties
  • bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/Messages.java
  • bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/messages.properties
  • releng/com.espressif.idf.target/com.espressif.idf.target.target
Additional comments (Suppressed): 14
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/wizard/pages/ManageToolsInstallationWizardPage.java (1)
  • 54-61: The import statement for LogMessagesThread has been moved from the old hunk to the new one. Ensure that this change does not affect the functionality of the code, as the order of import statements can sometimes matter in certain contexts.
bundles/com.espressif.idf.ui/META-INF/MANIFEST.MF (2)
  • 41-45: The new hunk adds two dependencies: org.eclipse.core.variables and org.eclipse.ltk.core.refactoring. Ensure that these dependencies are compatible with the rest of your project and that they don't introduce any breaking changes. Also, verify that the specified version for org.eclipse.core.variables is correct.

  • 49-54: New packages com.espressif.idf.ui.gcov and com.espressif.idf.ui.dialogs have been added to the exported packages list. Make sure that these packages are intended to be exposed to other bundles and that they contain only the classes and interfaces that should be publicly accessible.

bundles/com.espressif.idf.serial.monitor/src/com/espressif/idf/serial/monitor/core/IDFMonitor.java (1)
  • 190-196: The method handlePythonDependenciesInstall() is now being called instead of the previous handleWebSocketClientInstall(). Ensure that this change doesn't affect other parts of the codebase that rely on the WebSocket client being installed by this method. Also, verify that the new method correctly handles the installation of Python dependencies as expected.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/wizard/pages/InstallEspIdfPage.java (1)
  • 36-44: The import statements are rearranged, and there is no addition or removal of any imports. Ensure that the new arrangement doesn't violate your project's code style guidelines.
bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/dsf/Launch.java (3)
  • 22-28: The import statement for DsfExecutor has been added. Ensure that this new import does not conflict with any existing code and is used appropriately in the subsequent code.

  • 38-43: The import statement for LaunchConfigurationWorkingCopy from org.eclipse.debug.internal.core has been removed and replaced with ILaunchConfigurationWorkingCopy from org.eclipse.debug.core. This change could potentially affect the functionality of the code if methods specific to LaunchConfigurationWorkingCopy were being used. Please verify that this change doesn't break any existing functionality.

  • 74-91: As per the previous discussion, the getSession() and getDsfExecutor() methods have been overridden to make them publicly accessible. The changes are approved as they meet the requirement stated.

bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsInstallationHandler.java (3)
  • 574-580: The method handlePipDependenciesInstall() is called here, which now installs the gcovr Python package. Ensure that this new dependency does not conflict with other parts of the system and that it's compatible with the current Python environment.

  • 982-985: The method name has been changed from handleWebSocketClientInstall() to handlePipDependenciesInstall(). Verify that all calls to this method throughout the codebase have been updated to match the new method name.

  • 997-999: The gcovr package is added as a new Python dependency. Make sure that this package is compatible with the existing Python environment and doesn't introduce any conflicts with other dependencies.

bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/InstallToolsHandler.java (3)
  • 96-103: The task name for the monitor has been updated to reflect the new functionality of installing Python dependencies. Ensure that the corresponding message in Messages.InstallToolsHandler_InstallingPythonDepMsg is correctly set to indicate this change.

  • 219-225: The method handlePythonDependenciesInstall() has been introduced to handle the installation of Python dependencies, specifically websocket-client and gcovr. This is a change from the previous version where only websocket-client was installed. Make sure that the addition of gcovr does not introduce any compatibility issues with the existing system.

  • 264-278: The check for installed dependencies now includes gcovr along with websocket-client. If both are found, the function returns an OK status. If not, it attempts to install both. This is a change from the previous version where only websocket-client was checked and installed. Ensure that the installation process handles potential errors or failures when installing gcovr.

Copy link
Collaborator Author

@alirana01 alirana01 left a comment

Choose a reason for hiding this comment

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

updated self review after changes

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 0

Commits Files that changed from the base of the PR and between e9289a8 and 1076037.
Files ignored due to filter (6)
  • docs/images/GcovCodeCoverage/gcov-1.png
  • docs/images/GcovCodeCoverage/gcov-2.png
  • docs/images/GcovCodeCoverage/gcov-3.png
  • docs/images/GcovCodeCoverage/gcov-4.png
  • docs/images/GcovCodeCoverage/gcov-5.png
  • docs/images/GcovCodeCoverage/gcov-6.png
Files selected for processing (1)
  • README.md (2 hunks)
Files skipped from review due to trivial changes (1)
  • README.md

@coderabbitai
Copy link

coderabbitai bot commented Sep 19, 2023

Rate Limit Exceeded

You have exceeded the limit for the number of files or commits that can be reviewed per hour. Please wait 29 minutes and 59 seconds before requesting another review.

To re-trigger a review, convert this PR to a draft and then mark it as ready for review again. Alternatively, you can push a new commit to this PR.

@coderabbitai
Copy link

coderabbitai bot commented Sep 19, 2023

Rate Limit Exceeded

You have exceeded the limit for the number of files or commits that can be reviewed per hour. Please wait 29 minutes and 59 seconds before requesting another review.

To re-trigger a review, convert this PR to a draft and then mark it as ready for review again. Alternatively, you can push a new commit to this PR.

@alirana01 alirana01 changed the title [WIP] IEP-374: GCOV Reports view and creation IEP-374: GCOV Reports view and creation Sep 19, 2023
Copy link
Collaborator

@kolipakakondal kolipakakondal left a comment

Choose a reason for hiding this comment

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

Good work @alirana01 LGTM

Comment on lines 28 to 30
org.eclipse.embedcdt.core;visibility:=reexport,
org.eclipse.linuxtools.gcov.core,
org.eclipse.linuxtools.gcov.docs,
org.eclipse.linuxtools.gcov.launch,
org.eclipse.linuxtools.binutils,
org.eclipse.linuxtools.gprof,
org.eclipse.linuxtools.gprof.docs,
org.eclipse.linuxtools.gprof.launch,
org.eclipse.linuxtools.tools.launch.core,
org.eclipse.linuxtools.tools.launch.ui,
org.eclipse.linuxtools.dataviewers,
org.eclipse.linuxtools.dataviewers.charts,
org.eclipse.linuxtools.dataviewers.piechart,
org.eclipse.linuxtools.docker.core,
org.eclipse.linuxtools.docker.ui,
org.eclipse.linuxtools.profiling.docs,
org.eclipse.linuxtools.profiling.launch,
org.eclipse.linuxtools.profiling.ui,
org.eclipse.core.variables
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we add what is mandatory here?

<command
defaultHandler="com.espressif.idf.debug.gdbjtag.openocd.gcov.GcovDumpHandler"
id="com.espressif.idf.gcov.instant"
name="GCOV Instant">
Copy link
Collaborator

Choose a reason for hiding this comment

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

defaultHandler="com.espressif.idf.debug.gdbjtag.openocd.gcov.GcovDumpHandler"
id="com.espressif.idf.gcov.hardcoded"
name="GCOV Hard-coded">
</command>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Comment on lines +130 to +131
<unit id="org.eclipse.linuxtools.profiling.feature.group" version="0.0.0"/>
<unit id="org.eclipse.linuxtools.profiling.remote.feature.group" version="0.0.0"/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you check if we need all of them? Probably, we need only org.eclipse.linuxtools.gcov.feature.group

@kolipakakondal
Copy link
Collaborator

Hi @AndriiFilippov Please verify this

@kolipakakondal kolipakakondal added this to the v2.12.0 milestone Sep 28, 2023
@kolipakakondal kolipakakondal added enhancement New feature or request needs translation:CN Chinese translation is required labels Sep 28, 2023
@AndriiFilippov
Copy link
Collaborator

@alirana01 hi !

Tested under:
OS - Windows 10
ESP-IDF: v5.1.1

Able to generate reports using Generate Hard-coded GCOV Dump:
1
2
3
6

but getting the console error:
image

Also was not able to use "Generate Instant Runtime GCOV Dump". Even after restar OpenOCD or Reset the board by issuing the reset command via telnet like it mentioned https://github.com/espressif/esp-idf/blob/master/examples/system/gcov/README.md#openocd-out-of-sync

Getting error:

Info : [esp32.cpu0] Target halted, PC=0x400855F2, debug_reason=00000000
Info : Set GDB
 target to '
esp32.cpu0'
Info : [esp32.cpu1] 
Target halted, PC=0x400855F2, debug_reason=00000000
Info : Targets disconnected.
Info : [esp32.cpu0] Target halted
, PC=0x400855F2, debug_reason=00000000
Info : Set GDB targ
et to 'esp32.cpu0'
Info : [esp32.cpu1] Target halted, PC=0x400855F2, debug_reason=00000000

Thread 
0x400855f2 in esp_cpu_wait_for_intr () at C:/Users/AndriiFilippov/e/esp-idf-v5.1.1/components/esp_hw_support/cpu.c:121
Info : Total trace memory: 16384 bytes
Connect targets...
Info : Targets connected.
Error: Data timeout!
Error: Failed to poll target for gcov data (-4)!
Disconnect targets...
Info : [esp32.cpu0] Target halted, PC=0x400855F2, debug_reason=00000000
Info : Set GDB target to 'esp32.cpu0'
Info : [esp32.cpu1] Target halted, PC=0x400855F2, debug_reason=00000000
Info : Targets disconnected.

Also, there is identical icons for different actions:
image

@alirana01
Copy link
Collaborator Author

alirana01 commented Oct 9, 2023

@kolipakakondal I discussed this with Ivan seems like an issue from the OpenOCD I will discuss this with Erhan or Alexey Lipshin and will try to resolve this for now I dont think so its a good idea to push this with the release because the icons are also missing and I dont want to push it without proper confirmation from the other team

Further discussion here
https://mattermost.espressif.cn:6689/espressif/pl/xa9errkqt3npbnmtnjyshec7ya

@kolipakakondal kolipakakondal modified the milestones: v2.12.0, v2.13.0 Nov 27, 2023
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 2

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 1076037 and 2519d20.
Files selected for processing (3)
  • README_CN.md (15 hunks)
  • bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/gcov/GcovDumpHandler.java (1 hunks)
  • bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/gcov/GcovFileViewCommandHandler.java (1 hunks)
Additional comments: 9
README_CN.md (9)
  • 46-46: The addition of the "GCOV Code Coverage and Dump Generation" section to the table of contents is clear and correctly links to the new section further down in the document. This change effectively informs users about the new feature available in the ESP-IDF Eclipse plugin.
  • 61-63: The update to the prerequisites section, specifically the addition of the Eclipse IDE C/C++ Development Tool version requirement, is a necessary and helpful clarification for users setting up their development environment. It ensures that users install a compatible version of Eclipse, preventing potential compatibility issues.
  • 87-87: The instruction to select "Espressif IDF" from the list during the plugin installation process is clear and straightforward. This step is crucial for users to correctly install the ESP-IDF Eclipse plugin, and the explanation is concise and to the point.
  • 207-207: The addition of instructions for configuring the serial port output in Eclipse is a valuable update for users. It guides them through the process of setting up the ESP-IDF Serial Monitor, which is essential for debugging and monitoring the output of ESP-IDF projects.
  • 219-219: The update to the ESP-IDF Serial Monitor settings, specifically the addition of instructions for setting the character and line limits, is a useful enhancement. It allows users to customize the Serial Monitor according to their preferences, improving the usability of the tool.
  • 280-280: The addition of the "ESP-IDF Terminal" section provides users with instructions on how to open a local terminal window within Eclipse. This feature enhances the development experience by allowing users to access ESP-IDF terminal commands directly from the Eclipse environment.
  • 317-317: The update to the environment variables configuration section is crucial for ensuring that users correctly set up their development environment. It provides clear instructions on how to manually configure essential environment variables if they are not automatically set up by the ESP-IDF tool installer.
  • 415-429: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [402-426]

The addition of the troubleshooting section, including error logs, console view logs, and the Espressif IDF Toolchain Console, is a significant improvement. It provides users with guidance on how to diagnose and resolve issues that may arise during development, making the plugin more user-friendly and accessible.

  • 553-585: The detailed explanation of the GCOV Code Coverage and Dump Generation feature is a valuable addition to the README. It guides users through the process of generating and viewing GCOV reports within Eclipse, enhancing the plugin's functionality for code coverage analysis. However, it's important to ensure that the links provided in this section are accessible and lead to the correct resources.
Verification successful

The links provided in the GCOV Code Coverage and Dump Generation section are accessible. The first link redirects to a new URL, which is a common practice and does not hinder access to the intended content. The second link directly leads to the accessible README.md file. Therefore, the detailed explanation and the links are correctly guiding users through the process as intended.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Verify the accessibility of the provided links in the GCOV section.
curl -I https://github.com/espressif/esp-idf/blob/master/examples/system/gcov/
curl -I https://github.com/espressif/esp-idf/blob/master/examples/system/gcov/README.md

Length of output: 9051

Comment on lines +27 to +52
@Override
public Object execute(ExecutionEvent event) throws ExecutionException
{
IWorkbenchWindow window = PlatformUI.getWorkbench().getActiveWorkbenchWindow();
if (window == null)
{
throw new ExecutionException("Active workbench window cannot be retrieved");
}

IWorkbenchPage page = window.getActivePage();
if (page == null)
{
throw new ExecutionException("Active page cannot be retrieved");
}

try
{
GcovUtility.clearSelectedProject();
page.showView(GcovFileView.ID);
}
catch (PartInitException e)
{
Logger.log(UIPlugin.getDefault(), "Failed to initialize GcovFileView: " + e.getMessage(), e);
}
return null;
}
Copy link

Choose a reason for hiding this comment

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

The implementation of the execute method correctly retrieves the active workbench window and page, and then attempts to show the GcovFileView. However, there are a couple of areas that could be improved for robustness and maintainability:

  • Exception Handling: The method throws ExecutionException for null window or page, which is appropriate. However, consider logging these exceptions using the project's logging mechanism for better traceability.
  • Clearing Selected Project: Before showing the view, GcovUtility.clearSelectedProject() is called. Ensure this action does not have unintended side effects on other parts of the application that might rely on the previously selected project.

Comment on lines +58 to +157
@Override
public Object execute(ExecutionEvent event) throws ExecutionException
{
isInstant = event.getCommand().getId().equals(INSTANT_ID);
ILaunchManager launchManager = DebugPlugin.getDefault().getLaunchManager();
ILaunch[] launches = launchManager.getLaunches();
ILaunch launchActive = null;
for (ILaunch launch : launches)
{
if (!launch.isTerminated())
{
launchActive = launch;
break;
}
}

DsfServicesTracker dsfServicesTracker = ((Launch) launchActive).getDsfServicesTracker();
ICommandControlService commandControlService = dsfServicesTracker.getService(ICommandControlService.class);
DsfExecutor dsfExecutor = ((Launch) launchActive).getDsfExecutor();
IRunControl runControl = dsfServicesTracker.getService(IRunControl.class);
ICommandControlDMContext controlDmc = DMContexts.getAncestorOfType(commandControlService.getContext(),
ICommandControlDMContext.class);
IProcesses processControl = dsfServicesTracker.getService(IProcesses.class);
processControl.getProcessesBeingDebugged(controlDmc, new DataRequestMonitor<IDMContext[]>(dsfExecutor, null)
{
@Override
protected void handleSuccess()
{
executionDMContext = (IExecutionDMContext) (getData()[0]);
}
});
final ILaunch finalActiveLaunch = launchActive;
runControl.suspend(executionDMContext, new RequestMonitor(dsfExecutor, null)
{
@Override
protected void handleSuccess()
{
commandControlService.queueCommand(new CLICommand<>(commandControlService.getContext(),
isInstant ? "mon esp gcov dump" : "mon esp gcov"), new ImmediateDataRequestMonitor<>()
{
@Override
protected void handleSuccess()
{
runControl.resume(executionDMContext, new RequestMonitor(dsfExecutor, null));
ILaunchConfiguration launchConfig = finalActiveLaunch.getLaunchConfiguration();
Display.getDefault().asyncExec(() -> {
String projectName;
try
{
projectName = launchConfig.getAttribute(
ICDTLaunchConfigurationConstants.ATTR_PROJECT_NAME, StringUtil.EMPTY);

if (!projectName.isEmpty())
{
IProject project = ResourcesPlugin.getWorkspace().getRoot()
.getProject(projectName);
if (project != null && project.exists())
{
GcovUtility.setSelectedProject(project);
IWorkbenchWindow window = PlatformUI.getWorkbench()
.getActiveWorkbenchWindow();
if (window != null)
{
IWorkbenchPage page = window.getActivePage();
if (page != null)
{
try
{
GcovFileView gcovFileView = (GcovFileView) page
.showView(GcovFileView.ID);
page.activate(gcovFileView);

}
catch (PartInitException e)
{
Logger.log(e);
}
}
}
}
}
}
catch (CoreException e)
{
Logger.log(e);
}
});
}

@Override
protected void handleError()
{
Logger.log("Error Occurred while running dump comand resuming debug operation");
runControl.resume(executionDMContext, new RequestMonitor(dsfExecutor, null));
}
});
}
});
return null;
}
Copy link

Choose a reason for hiding this comment

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

The execute method in GcovDumpHandler performs several critical operations to handle gcov dump commands. Here are some areas for improvement and considerations:

  • Launch Selection: The method selects the first non-terminated launch it finds. This approach might not always select the correct or expected launch if multiple are active. Consider implementing a more precise selection mechanism.
  • Service Retrieval and Null Checks: Services are retrieved from the DsfServicesTracker without null checks. Adding null checks before using these services can prevent potential NullPointerExceptions.
  • UI Thread Operations: The method updates the UI from within the Display.getDefault().asyncExec block. Ensure that all UI-related operations are correctly encapsulated within this block to avoid SWT thread access violations.
  • Error Handling: The method logs errors but does not inform the user of failures directly. Consider improving user feedback for errors, especially for operations like runControl.resume in the handleError block of the command execution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request needs translation:CN Chinese translation is required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants