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 i2c dump filtering #60301

Merged
merged 6 commits into from Sep 6, 2023

Conversation

barnas-michal
Copy link
Collaborator

This PR changes format of the i2c messages dump, to align the messages for easier comparison, and in case of device emulators, it's using the target device name instead of generic "emul".
It also adds a possibility to filter which devices transactions should be dumped. If someone is debugging the i2c communication, the log being flooded with other devices' transactions make it hard to debug. The logged devices
should be specified in the device-tree using the compatible string.

Example of device-tree node:
i2c-dump-filter {
compatible = "zephyr,i2c-dump-filter";
devices = < &display0 >, < &sensor3 >;
};

galak
galak previously requested changes Jul 12, 2023
Copy link
Collaborator

@galak galak left a comment

Choose a reason for hiding this comment

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

I'm not fan of this as a devicetree node. Maybe a property under a controller or something.

@barnas-michal
Copy link
Collaborator Author

I'm not fan of this as a devicetree node. Maybe a property under a controller or something.

I am not sure if I would be able to easily iterate over all devices with some property set. It would also require to add the optional property to all possible i2c devices.
I know that this node doesn't represent any physical device, but I think it is the easiest and most readable approach to this functionality.

@tmon-nordic
Copy link
Contributor

I know that this node doesn't represent any physical device, but I think it is the easiest and most readable approach to this functionality.

I agree that this is the easiest approach. However, there is constantly a disagreement (not only in Zephyr, but also in Linux kernel) what constitutes a devicetree and what does not. Linux kernel tends to be stricter about not using devicetree for configuration purposes.

Unfortunately I cannot find my older comments about the same issue, but this is perfect example of a place where some "devicetree-like-configuration" would make sense. Maybe we should group such use cases into some issue?

teburd
teburd previously approved these changes Jul 14, 2023
Copy link
Collaborator

@teburd teburd left a comment

Choose a reason for hiding this comment

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

I like the way this works.

I was under the impression that zephyr compatibles were understood to be zephyr/virtual-ish like devices and configuration.

I don't see how this would easily be done in Kconfig. It wants structured data as configuration and DT is the tool Zephyr has for that.

aaronemassey
aaronemassey previously approved these changes Jul 14, 2023
Copy link
Member

@aaronemassey aaronemassey left a comment

Choose a reason for hiding this comment

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

@semihalf-barnas-michal

I know that this node doesn't represent any physical device, but I think it is the easiest and most readable approach to this functionality.

@tmon-nordic

I agree that this is the easiest approach.

In my opinion, this is really similar to the first acknowledged type of exception scenario listed in Zephyr's docs comparing DeviceTree with KConfig. So it's acceptable to me as devicetree mechanism.

keith-zephyr
keith-zephyr previously approved these changes Jul 17, 2023
Copy link
Contributor

@keith-zephyr keith-zephyr left a comment

Choose a reason for hiding this comment

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

Non-blocking nits from me.

drivers/i2c/Kconfig Outdated Show resolved Hide resolved
drivers/i2c/Kconfig Outdated Show resolved Hide resolved
@keith-zephyr
Copy link
Contributor

I'm not fan of this as a devicetree node. Maybe a property under a controller or something.

I am not sure if I would be able to easily iterate over all devices with some property set. It would also require to add the optional property to all possible i2c devices. I know that this node doesn't represent any physical device, but I think it is the easiest and most readable approach to this functionality.

The GPIO hogs driver provides an example for iterating over all devicetree nodes that have the gpio-controller property. Something similar could be done here by adding properties to the i2c-controller.yaml base class. But considering this is a debug feature I like the single devicetree node that collects up the list of I2C devices to log.

@teburd
Copy link
Collaborator

teburd commented Jul 31, 2023

@semihalf-barnas-michal Perhaps it would be beneficial to document this debug feature in the i2c bus docs somewhere, as an opt in feature to dump messages, or dump messages with filtering abilities using a dts node.

At the moment someone would need to read the source code to understand how to use this feature.

@barnas-michal
Copy link
Collaborator Author

barnas-michal commented Aug 3, 2023

in the i2c bus docs somewhere

I've extracted some of the debugging documentation to another file and added the documentation about this feature there. If you do not like it, I can revert and just add the documentation to the i2c doc file.
I see that there is another file, services/debug which may be in a conflict to my extraction... However I am not sure if the I2C documentation is suitable for this, since it is very long and mostly describing the I2C interface and API...

@barnas-michal barnas-michal force-pushed the mb-i2c-dump-filtering branch 2 times, most recently from 691a8db to cdcc15d Compare August 3, 2023 13:26
@teburd
Copy link
Collaborator

teburd commented Aug 3, 2023

in the i2c bus docs somewhere

I've extracted some of the debugging documentation to another file and added the documentation about this feature there. If you do not like it, I can revert and just add the documentation to the i2c doc file. I see that there is another file, services/debug which may be in a conflict to my extraction... However I am not sure if the I2C documentation is suitable for this, since it is very long and mostly describing the I2C interface and API...

Great thank you, I think as long as there's some documentation somewhere on it, it will be helpful. If it turns out to be in the wrong place that's easily fixed.

doc/develop/debug/index.rst Outdated Show resolved Hide resolved
doc/develop/debug/index.rst Outdated Show resolved Hide resolved
doc/develop/debug/index.rst Outdated Show resolved Hide resolved
doc/develop/debug/index.rst Outdated Show resolved Hide resolved
doc/develop/debug/index.rst Outdated Show resolved Hide resolved
doc/develop/debug/index.rst Outdated Show resolved Hide resolved
doc/develop/debug/index.rst Outdated Show resolved Hide resolved
doc/develop/debug/index.rst Outdated Show resolved Hide resolved
This commit changes the parameter of i2c_dump_msgs function from
string name to pointer to the device structure.
It allows for comparison of device pointers and allow to use
the printed device name in i2c shell commands.

Signed-off-by: Michał Barnaś <mb@semihalf.com>
This commit changes the format of printed messages to align the
following strings and make it more readable.

Signed-off-by: Michał Barnaś <mb@semihalf.com>
This commit adds option to dump i2c messages of only specified
devices. It makes it easier to debug communication of specific
i2c device instead of logging all i2c communication.
The filter of devices is specifiec in device-tree using the
node with "zephyr,i2c-dump-filter" compatible string.

Example of device-tree node:
i2c-dump-filter {
	compatible = "zephyr,i2c-dump-filter";
	devices = < &display0 >, < &sensor3 >;
};

Signed-off-by: Michał Barnaś <mb@semihalf.com>
Fix text that should be linking to the URLs but was missing
the link reference and remove the obsolete links.

Signed-off-by: Michał Barnaś <mb@semihalf.com>
Move the chapters about Application Debugging and
Debugging using Eclipse to another file for easier search and
to allow more debugging chapters to be inserted here.

Signed-off-by: Michał Barnaś <mb@semihalf.com>
Add the documentation about I2C_DUMP_MESSAGES and about the
I2C_DUMP_MESSAGES_FILTER in the develop/debug/index.rst doc file.

Signed-off-by: Michał Barnaś <mb@semihalf.com>
Copy link
Collaborator

@teburd teburd left a comment

Choose a reason for hiding this comment

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

Some of the single quoted additions to the docs don't make much sense to me at a quick glance

doc/develop/application/index.rst Show resolved Hide resolved
doc/develop/modules.rst Show resolved Hide resolved
@carlescufi carlescufi merged commit 9b2b964 into zephyrproject-rtos:main Sep 6, 2023
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants