Add HIL-TDD guidance documentation for PoKeysHal driver - #137
Merged
Conversation
- Introduced TDD Guidance for PokeysHal1.md and PokeysHal2.md to outline HIL-TDD principles and workflows. - Established a clear separation between unit tests and HIL tests, emphasizing the importance of hardware verification. - Documented the correct workflow for HIL-assisted Unit TDD, including the use of HIL observations as test oracles. - Revised test layering to distinguish between unit tests, HAL integration tests, and HIL acceptance tests. - Created a detailed HIL fixture contract and proposed repository structure for HIL tests. - Added comprehensive HIL guidance thoughts to ensure clarity on the HIL testing process and its integration with existing workflows.
There was a problem hiding this comment.
Pull request overview
Adds Hardware-in-the-Loop (HIL) / TDD guidance documentation for the PoKeysHal driver, describing recommended test layering (unit vs HAL smoke vs HIL fixture vs machine integration) and capturing a proposed PoKeys57E loopback fixture contract.
Changes:
- Added multiple guidance documents describing HIL-assisted unit TDD workflows and test-layer separation.
- Added PoKeys57E loopback wiring / pin-mode notes intended to support HIL fixture design.
- Added two “recommendation” documents outlining a proposed repo structure and process for HIL testing artifacts.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/HIL_Setup_4_TDD/TDD_HILguidance_thoughts.md | High-level HIL/TDD guidance and proposed instruction/workflow draft. |
| docs/HIL_Setup_4_TDD/TDD Guidance for PokeysHal1.md | HIL-assisted unit TDD workflow and examples of “HIL-derived oracle” unit tests. |
| docs/HIL_Setup_4_TDD/TDD Guidance for PokeysHal2.md | Recommended test layering and proposed split between basic HIL and integration tests. |
| docs/HIL_Setup_4_TDD/PoKeys57E pin modes.md | Fixture wiring/pin-mode capability notes and reuse ideas for PoKeys57E. |
| docs/HIL_Setup_4_TDD/HIL_TDD_guidance_suggestion1.md | Proposed 4-layer artifact structure and workflow recommendations for HIL testing. |
| docs/HIL_Setup_4_TDD/HIL_moduletests_thoughts.md | Second copy of the same recommendation/workflow content (currently duplicated). |
| @@ -0,0 +1,377 @@ | |||
| Yes. The existing `Pokeys57E_SimPins.hal` is a good HIL fixture. It already documents the physical loopbacks and uses `wcomp` comparators to turn simulated axis positions into physical PoKeys output pins, which are wired back into PoKeys inputs. That means you can test the full chain: | |||
| -> LinuxCNC HAL/motion-facing signal | ||
| ``` | ||
|
|
||
| The existing config uses `DEVICE_ID=27295` and `ApplyIniSettings=1`, so the HIL profile should lock to that board unless explicitly overridden. The machine config loads `Pokeys_DM542_XXYZ_mill.hal`, `pokeys_homing.hal`, and then `Pokeys57E_SimPins.hal`, so the current setup already treats the sim pins as part of the HAL stack. |
| | Machine integration | Full `DM542_XXYZ_mill` config, homing, sim switches, LinuxCNC motion wiring | **Yes** | Yes | | ||
| | Full motion validation | Motors/drives/spindle connected | Yes | Yes | | ||
|
|
||
| The existing `Pokeys57E_SimPins.hal` is useful evidence of the wiring harness: it documents the loopbacks for digital switch simulation, PWM-to-analog, and pulse-engine-to-encoder paths. But its `wcomp` logic and position-derived simulated switches are already **machine-behavior simulation**, not the minimal primitive HIL test. |
|
|
||
| Unless the test explicitly belongs to `integration/`. | ||
|
|
||
| The full config currently loads `Pokeys_DM542_XXYZ_mill.hal`, `pokeys_homing.hal`, and `Pokeys57E_SimPins.hal`; that makes it a multi-file machine configuration, therefore integration scope. |
| Physical pin 28 -> pokeys.0.pins.27 | ||
| ``` | ||
|
|
||
| This is already visible in `Pokeys57E_SimPins.hal`: physical pins 23–26 are driven through `digout.22`–`digout.25`, with `PinFunction 4` selecting digital-output mode. |
Comment on lines
+1
to
+5
| # Recommendation | ||
|
|
||
| Use **four separate layers**, each with a narrow responsibility: | ||
|
|
||
| | Artifact | Purpose | Recommendation | |
Comment on lines
+425
to
+426
| [1]: https://docs.github.com/en/copilot/concepts/agents/about-agent-skills?utm_source=chatgpt.com "About agent skills - GitHub Docs" | ||
| [2]: https://docs.github.com/en/copilot/how-tos/copilot-on-github/customize-copilot/customize-cloud-agent/create-custom-agents?utm_source=chatgpt.com "Creating custom agents for Copilot cloud agent - GitHub Docs" |
Comment on lines
+425
to
+426
| [1]: https://docs.github.com/en/copilot/concepts/agents/about-agent-skills?utm_source=chatgpt.com "About agent skills - GitHub Docs" | ||
| [2]: https://docs.github.com/en/copilot/how-tos/copilot-on-github/customize-copilot/customize-cloud-agent/create-custom-agents?utm_source=chatgpt.com "Creating custom agents for Copilot cloud agent - GitHub Docs" |
|
|
||
| ### 1. Pin-index mapping | ||
|
|
||
| The existing HAL file confirms that physical Pin 23 is driven through `pokeys.0.digout.22.out`, physical Pin 24 through `digout.23.out`, and so on. That should become a unit test for the mapping helper, not a scattered assumption. |
|
|
||
| ### 2. PEv2 switch mapping | ||
|
|
||
| The INI maps X limit-minus to Pin 28 and X home to Pin 29. Positive limit is disabled. So the unit test should encode exactly that: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Brief description of the changes in this PR.
Related Issues
Fixes #
Implements #
Part of #
Type of Change
Traceability
Testing
Describe how this was tested:
bash test_compile.shpassesmake -f Makefile.noqmakebuilds without errorshalrun/loadusr)halrun/loadrt)Real-Time Safety Checklist (if RT code changed)
malloc/freein real-time thread pathsO_NONBLOCK)mlockallused where requiredhal_s32_t,hal_bit_t,hal_float_t)Checklist