Fix: PUSH flag handling for exact multiples of 480 in DDP #833
Conversation
WalkthroughThe update introduces a modification to the Changes
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? TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Additionally, you can add CodeRabbit Configration File (
|
🔍 Existing Issues For ReviewYour pull request is modifying functions with the following pre-existing issues: 📄 File: ledfx/devices/ddp.py
Did you find this useful? React with a 👍 or 👎 |
for more information, see https://pre-commit.ci
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- ledfx/devices/ddp.py (1 hunks)
Additional comments: 2
ledfx/devices/ddp.py (2)
- 98-98: The addition of the
lastparameter in thesend_packetcall within thesend_outmethod is a crucial change. This parameter is used to determine if the current packet is the last in a sequence, which in turn decides whether the PUSH flag should be set. This logic is essential for ensuring that displays update correctly when the pixel count is an exact multiple of 480. The implementation appears to correctly handle the calculation of whether a packet is the last one by comparing the current packet index (i) with the total number of packets (packets). This is a well-thought-out approach to solving the issue at hand.- 102-107: The modification to the
send_packetmethod to include thelastparameter and use it to conditionally set the PUSH flag in the packet header is the key fix for the issue described. The logic to set the PUSH flag only on the last packet (DDPDevice.PUSH if last else 0) is correctly implemented. This change ensures that the display updates correctly for setups with exactly 480 pixels, addressing the previously identified problem. It's important to note that this approach maintains compatibility with existing behavior by preserving the logic that sets the PUSH flag on the last packet of a frame, extending this treatment to scenarios involving exact multiples of 480 pixels. The implementation is concise and directly addresses the problem without introducing unnecessary complexity.
With the prior code with exact multiples of 480 pixels counts the PUSH flag would never be set
Treating it as a display now flag, although its definition is not quite definitive in this regard
http://www.3waylabs.com/ddp/
This addresses the issues discussed at
https://discord.com/channels/469985374052286474/1216471237090480259
Where a WLED setup with exactly 480 pixels would not update, although the WLED peek does work
Also tested on the Falcon Player Pro as a DDP endpoint, which does not care about the PUSH flag at all.
To gain most benefit from prior maturity, preserved prior expression of sending push flag on last packet of a frame, now also for if it is an exact multiple of 480 pixels.
Tested as not working / working on a 512 WLED endpoint with ledfx device set to 480 which could be seen to only work for Peek and not pixels prior to fix
Now shows full effect at all times.
Regression tested on 16K FPP as well.
Summary by CodeRabbit