Skip to content

Fix bitstream metadata parser path handling - #1670

Open
graemef681 wants to merge 1 commit into
Xilinx:pynq-nextfrom
graemef681:fix-metadata-parser-bitstream-handling
Open

graemef681 wants to merge 1 commit into
Xilinx:pynq-nextfrom
graemef681:fix-metadata-parser-bitstream-handling

Conversation

@graemef681

Copy link
Copy Markdown

Summary

This PR fixes bitstream format detection when .bit and .xsa files with the same stem exist in the same directory.

It also converts Path objects to strings when passing .hwh and .xsa paths to the Metadata API.

Problem

BitstreamHandler.is_xsa() currently replaces the requested file's suffix with .xsa and checks whether that neighbouring file exists.

In simplified form, the existing check is:

xsa_file = requested_path.with_suffix(".xsa")
return xsa_file.exists()

This asks:

Does an .xsa file with this name exist at this path?

For example, requesting:

/path/design.bit

causes the code to check whether this file exists:

/path/design.xsa

If both files exist, the requested design.bit is incorrectly classified as an XSA.

The intended check is:

return requested_path.suffix == ".xsa"

This checks whether the exact path requested by the caller refers to an .xsa or .bit file. A neighbouring same-stem file can no longer change the classification of the requested file.

Incorrect classification can select the XSA metadata path even though the caller explicitly requested the bitstream.

Change

is_xsa() now checks the suffix of the requested path itself:

  • design.bit is classified as a bitstream.
  • design.xsa is classified as an XSA.
  • A same-stem neighbouring file does not affect the result.

RemoteBitstreamHandler inherits is_xsa() from BitstreamHandler. The same bug therefore affects remote bitstream handling without a separate implementation in remote_device.py. Correcting the base method fixes format classification for both embedded and remote handlers.

The accompanying changes in remote_device.py support the corrected parser flow. Once the requested file has been classified, its .hwh or .xsa path is converted from Path to str before being passed to the Metadata API. This keeps the remote parser path consistent with the embedded implementation and with the input type expected by Metadata.

A regression test covers the case where same-stem .bit and .xsa files both exist.

Testing

This test does not require an FPGA or PYNQ board.

The PR contains one commit on top of the current pynq-next revision. This allows the committed regression test to be run against both the previous and corrected implementations.

Create an environment from a checkout of the PR branch:

python -m venv /tmp/pynq-parser-test
source /tmp/pynq-parser-test/bin/activate
python -m pip install --upgrade pip
python -m pip install -e . pytest pytest-cov pydantic

pytest-cov is required because the repository's pytest.ini automatically enables coverage with --cov=pynq.

Current pynq-next behaviour

Temporarily restore the pre-PR implementations while retaining the regression test:

git restore --source=HEAD^ -- \
  pynq/pl_server/embedded_device.py \
  pynq/pl_server/remote_device.py

Run the regression test:

python -m pytest -q \
  tests/test_bitstream.py::test_bitstream_handler_uses_requested_suffix_for_xsa

Expected result: the test fails at:

assert BitstreamHandler(bit_path).is_xsa() is False

The current implementation returns True because a same-stem testbitstream.xsa exists beside the requested .bit file.

This PR

Restore the PR implementations:

git restore --source=HEAD -- \
  pynq/pl_server/embedded_device.py \
  pynq/pl_server/remote_device.py

Run the same test:

python -m pytest -q \
  tests/test_bitstream.py::test_bitstream_handler_uses_requested_suffix_for_xsa

Expected result:

1 passed

The test passes because each requested file is classified using its own suffix.

Confirm that the checkout has been fully restored:

git status --short

This command should produce no output.

Signed-off-by: Graeme Fitzpatrick graemef681@gmail.com

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant