Skip to content

Conversation

@ndrs-pst
Copy link
Contributor

@ndrs-pst ndrs-pst commented Oct 28, 2025

This PR is dedicated to improving devmem_service.c through the following commits:

  • Use shell_print where applicable
  • Utilize sys_put_le(16|32|64) in memory_dump

To improve code clarity, use `shell_print` in place of `shell_fprintf`
with `SHELL_NORMAL` where appropriate.

Signed-off-by: Pisit Sawangvonganan <pisit@ndrsolution.com>
@zephyrbot zephyrbot added the area: Shell Shell subsystem label Oct 28, 2025
@ndrs-pst
Copy link
Contributor Author

Ping @jakub-uC / @ycsin for any feedback.

hex_data[data_offset] = (uint8_t)value;
value >>= 8;
hex_data[data_offset + 1] = (uint8_t)value;
sys_put_be16(value, &hex_data[data_offset]);
Copy link
Member

Choose a reason for hiding this comment

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

static inline void sys_put_be16(uint16_t val, uint8_t dst[2])
{
	dst[0] = val >> 8;
	dst[1] = val;
}

You probably should use sys_put_le16() to maintain the same endianness?

static inline void sys_put_le16(uint16_t val, uint8_t dst[2])
{
	dst[0] = val;
	dst[1] = val >> 8;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for my misinterpretation. It seems that devmem dump will always output memory in Little-Endian byte order, regardless of the underlying architecture. The correct one is as follows:

			case 16:
				value = sys_read16(addr + data_offset);
				sys_put_le16(value, &hex_data[data_offset]);
				break;
			case 32:
				value = sys_read32(addr + data_offset);
				sys_put_le32(value, &hex_data[data_offset]);
				break;
#ifdef CONFIG_64BIT
			case 64:
				value = sys_read64(addr + data_offset);
				sys_put_le64(value, &hex_data[data_offset]);
				break;
#endif /* CONFIG_64BIT */

break;
case 16:
value = sys_le16_to_cpu(sys_read16(addr + data_offset));
value = sys_read16(addr + data_offset);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe you should add big endian support for this shell command instead of converting it to big endian?

Copy link
Contributor

Choose a reason for hiding this comment

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

good point

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe you should add big endian support for this shell command instead of converting it to big endian?

Thanks for the suggestion! For now, I'm focusing on streamlining the existing codebase, so I'll hold off on adding big-endian support in this PR.

Replaces direct byte shifting and assignment with `sys_put_le16`,
`sys_put_le32`, and `sys_put_le64` to simplify the `memory_dump` function.

Signed-off-by: Pisit Sawangvonganan <pisit@ndrsolution.com>
@ndrs-pst ndrs-pst force-pushed the pr_shell_devmem_service_2025-10-28 branch from bf9176d to 44a2a2b Compare November 21, 2025 15:09
@sonarqubecloud
Copy link

@ndrs-pst
Copy link
Contributor Author

ndrs-pst commented Dec 9, 2025

Ping @ycsin.

Copy link
Member

@ycsin ycsin left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

@jhedberg jhedberg merged commit c536579 into zephyrproject-rtos:main Dec 11, 2025
28 checks passed
@ndrs-pst ndrs-pst deleted the pr_shell_devmem_service_2025-10-28 branch December 11, 2025 11:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: Shell Shell subsystem

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants