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 function of allocating fifosize on adc separately #13665

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

Conversation

zhangkai25
Copy link
Contributor

Summary

allocating fifosize on demand to optimize memory ,no need to seting adc_fifosize unified

Impact

Allocate fifosize on demand to optimize memory & Fix the situation where af_channel is still being read after it is released

Testing

Signed-off-by: zhangkai25 <zhangkai25@xiaomi.com>
@github-actions github-actions bot added the Size: S The size of the change in this PR is small label Sep 27, 2024
@nuttxpr
Copy link

nuttxpr commented Sep 27, 2024

[Experimental Bot, please feedback here]

No, this PR does not meet all the NuttX requirements.

Here's why and how to improve it:

  • Missing Information in Summary:

    • Why is the change necessary? Clearly state the problem the current fixed FIFO size causes. For example, "The current fixed adc_fifosize leads to wasted memory when applications don't require a large buffer, especially on memory-constrained devices."
    • What functional part is changed? Be specific. Mention the ADC driver or subsystem.
    • How does it work? Briefly explain the on-demand allocation mechanism. When is memory allocated/freed?
  • Incomplete Impact Section:

    • Impact on User: Do users need to change their application code to use this new feature? Will any ADC APIs change?
    • Impact on Build: Are there any new configuration options that users might need to set?
    • Impact on Hardware: Is this change specific to certain ADC hardware or architectures?
    • Impact on Documentation: Documentation updates are almost always needed for code changes. State that documentation will be updated or provide a link to a separate documentation PR.
  • Missing Testing Details:

    • Build Hosts: List the operating systems and compilers you used to build and test the change.
    • Target(s): Specify the exact architectures and boards you tested on (e.g., "sim:qemu-arm", "ARM: STM32F4DISCOVERY").
    • Testing Logs: The logs are empty. Provide relevant snippets showing:
      • The problem before the change (e.g., excessive memory usage, errors related to the af_channel issue).
      • The successful operation after the change (e.g., reduced memory usage, correct af_channel behavior).

Example of an Improved Summary:

"This PR optimizes memory usage in the ADC driver by allocating the FIFO buffer size on demand. Previously, a fixed adc_fifosize was used, leading to potential memory waste. The change introduces a dynamic allocation mechanism that allocates memory when an ADC channel is opened and frees it upon channel closure. This addresses the inefficiency of the previous approach and ensures optimal memory utilization."

drivers/analog/adc.c Outdated Show resolved Hide resolved
/* Malloc for af_channale and af_data */

fifosize = fifo->af_fifosize;
if (fifosize == 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

If fifo->af_fifosize is nonzero (I suppose this is set from BSP/arch during ADC initialization, right?), the BSP/arch is also responsible for allocating af_channel and af_data? What if this doesn't happen? I would let adc_register do all the allocation and just set default size if fifo->af_fifosize = 0.

Copy link
Contributor

Choose a reason for hiding this comment

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

how about skip allocation of af_channel/af_data if it isn't zero?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When fifosize is not 0, af_data and channel are passed from the lower half of the vendor.The reason why fifosize is equal to 0 is that the vendor layer does not define the fifosize macros of each ADC device separately. Therefore, nuttx still uses CONFIG_ADC_FIFOSIZE uniformly. If the fifosize of each ADC is defined separately on the vendor side, then the judgment of if (fifosize == 0) will not be entered.

Copy link
Contributor

Choose a reason for hiding this comment

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

My point was more about what happens if the lower half does not initialize af_data and af_channel correctly. There should at least be some check at the end of adc_register that controls whether both arrays are allocated imho.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks,I understand what you mean, and will make some changes

Copy link
Contributor

Choose a reason for hiding this comment

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

where the change?

…leased

Signed-off-by: zhangkai25 <zhangkai25@xiaomi.com>
@@ -771,7 +771,8 @@ int adc_register(FAR const char *path, FAR struct adc_dev_s *dev)
return -ENOMEM;
}

fifo->af_data = kmm_malloc(fifo->af_fifosize * 4);
fifo->af_data = kmm_malloc(fifo->af_fifosize *
Copy link
Contributor

Choose a reason for hiding this comment

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

merge to previous patch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Size: S The size of the change in this PR is small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants