search

zephyrproject-rtos / zephyr

Primary Git Repository for the Zephyr Project. Zephyr is a new generation, scalable, optimized, secure RTOS for multiple hardware architectures.
https://docs.zephyrproject.org
Apache License 2.0
9.83k stars 6.06k forks source link

MediaTek 8195 Audio DSP, take 2 #72766

Open andyross opened 2 weeks ago

andyross commented 2 weeks ago

This is getting hot again. Rebase, update for changes and push again. Will split out the early patches as separate PRs while this is in review.

andyross commented 2 weeks ago

Split-out platform-layer PRs in #72768 #72769 and #72771

Please review that content there, I'll remove from here as they merge.

andyross commented 2 weeks ago

Stick a DNM on temporarily now that I see this has some +1's just so it doesn't merge before the dependee PRs

andyross commented 1 week ago

Noting here also: #72771 and #72768 still need some review love.

andyross commented 1 week ago

Rebase now that all the dependee PRs have merged. This is now a pure "new platform" PR that should be able to go in in isolation, presuming I didn't break anything in the rebase. Please re-review.

andyross commented 1 week ago

Spun on kconfig linting a bit, should be good now. The two pylint warnings are false positives: in that particular construct an iterate has no advantages.

andyross commented 3 days ago

Ping for merge? As mentioned, the pylint warning is contra-idiomatic (this is a bytewise copy over an mmap region, doing it with python enumerate() would be needlessly confusing). Also there's a doc build failure which seems to be a CI glitch?

andyross commented 1 day ago

To clarify the pylint issue, which provoked some discussion on discord, pylint has a test where it detects where you are doing "for i in range(len(some_iterable)) and using both the index i and the element some_iterable[i]. And it suggests that you use enumerate() instead, which returns both.

Which is all well and good for the typical lists and dics with which the authors assumed it would be used. But in the cases it's flagging I'm iterating into two arrays (bytes and mmap objects) with the same index, and the asymmetry is confusing and weird. It wants to turn a trivially obvious bytewise copy that looks like this:

for i in range(len(input)):
    output[i] = input[i]

into this asymmetric silliness, which obscures the actual operation that's happening:

for i, elem in enumerate(input):
    output[i] = elem

It's just an inappropriate warning. In fact it's not even at "[W]arning" level, it's a "[C]onvention" message, asking me to "consider" using the alternative construction. And I did consider it! And I won't, because it's silly.

cfriedt commented 8 hours ago

Maybe @nashif?