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

Enhance HITL test #2209

Merged
merged 16 commits into from
Aug 9, 2023
Merged

Enhance HITL test #2209

merged 16 commits into from
Aug 9, 2023

Conversation

HiFiPhile
Copy link
Collaborator

@HiFiPhile HiFiPhile commented Aug 6, 2023

Describe the PR
Add HITL testing script for newly added boards #2194

Currently support cdc_dual_ports, cdc_msc, dfu, dfu_runtime, more tests will be added later on.

For my instance use python test/hitl/hitl_test.py hitl_hfp.json

I'm not familiar with Github, @hathach could you take a look for CI integration ?

Additional context

Actually it catched something !
My lpcxpresso54608 (even modified 54628 to 54608 in board.mk) failed with cdc_msc test, msc block doesn't show up :

[242341.797154] usb 4-1.7: new high-speed USB device number 89 using xhci_hcd
[242341.901761] usb 4-1.7: New USB device found, idVendor=cafe, idProduct=4003, bcdDevice= 1.00
[242341.901765] usb 4-1.7: New USB device strings: Mfr=1, Product=2, SerialNumber=3
[242341.901767] usb 4-1.7: Product: TinyUSB Device
[242341.901768] usb 4-1.7: Manufacturer: TinyUSB
[242341.901768] usb 4-1.7: SerialNumber: 0123456789ABCDEF
[242341.904327] cdc_acm 4-1.7:1.0: ttyACM2: USB ACM device
[242341.904686] usb-storage 4-1.7:1.2: USB Mass Storage device detected
[242341.904901] scsi host7: usb-storage 4-1.7:1.2
[242343.001409] usb 4-1.7: reset high-speed USB device number 89 using xhci_hcd
[242343.102100] cdc_acm 4-1.7:1.0: ttyACM2: USB ACM device
[242343.205356] usb 4-1.7: reset high-speed USB device number 89 using xhci_hcd
[242343.306000] cdc_acm 4-1.7:1.0: ttyACM2: USB ACM device
[242343.401299] usb 4-1.7: reset high-speed USB device number 89 using xhci_hcd
[242343.505975] cdc_acm 4-1.7:1.0: ttyACM2: USB ACM device
[242343.605305] usb 4-1.7: reset high-speed USB device number 89 using xhci_hcd
[242343.705934] cdc_acm 4-1.7:1.0: ttyACM2: USB ACM device

Log:
msc.zip

@hathach
Copy link
Owner

hathach commented Aug 7, 2023

@HiFiPhile yeah, I notice MSC is not showed up on your VM. I thought it is due your VM not enable auto mount block device, I remembered we could find it /dev/sda. I guess we have to manual mount ? I will check out the PR later.

@HiFiPhile
Copy link
Collaborator Author

I thought it is due your VM not enable auto mount block device

It's true. I set udev alias, it will show up with something like /dev/blkUSB_57323020.02

@hathach
Copy link
Owner

hathach commented Aug 7, 2023

I thought it is due your VM not enable auto mount block device

It's true. I set udev alias, it will show up with something like /dev/blkUSB_57323020.02

we should actually read the contents of README.TXT to verify that it works. Give me a bit of time. I will add manual mount to this PR when possible.

@HiFiPhile
Copy link
Collaborator Author

we should actually read the contents of README.TXT to verify that it works.

Currently I read the content directly from block device.

readme = \
b"This is tinyusb's MassStorage Class demo.\r\n\r\n\
If you find any bugs or get any questions, feel free to file an\r\n\
issue at github.com/hathach/tinyusb"
assert data[0x600:0x600 + len(readme)] == readme, 'Block wrong data'
print('cdc_msc test done')

I was too lazy to do mounting rightly to take care permissions and when multiples devices running msc_cdc example.

@HiFiPhile
Copy link
Collaborator Author

Now it read README.TXT from mounted device.

@hathach
Copy link
Owner

hathach commented Aug 8, 2023

Now it read README.TXT from mounted device.

thank you perfect, I will check this out soon enough. Though one thing pop up in my mind. I think at the end of the CI. We should flash board_test example. This will disable the usb on the tested board. Making testing the next board easier, at least it will more forgiving in our test script.

@hathach
Copy link
Owner

hathach commented Aug 8, 2023

lpc54 has an issue with receiving 31 bytes for msc command and sending 13 for msc status. Somehow when the transfer is complete, it is only 30 bytes (1 residue) and 12 bytes. Seem like the interrupt triggered a bit early or something. I have spent something troubleshoot, but not quite sure what is wrong. We probably need to skip the lpc54 for now, lpc55 has no issue with this though, strange.

@HiFiPhile
Copy link
Collaborator Author

We probably need to skip the lpc54 for now

Yes we can fix it later.

@hathach
Copy link
Owner

hathach commented Aug 8, 2023

@HiFiPhile everything looks great and the python test script is way better than my shell script and easier to work with. Though I think we may have to modify it a bit to work better with ci. I will try to wrap this up tomorrow.

@HiFiPhile
Copy link
Collaborator Author

I was looking around for lpc54 issue and found there are 17 USB issues in errata sheet, that's a lot...

Maybe it's related:

* USB.1: In USB high-speed device mode, the NBytes field does not decrement after BULK OUT transfer
Introduction:
The LPC546xx. device family includes a USB high-speed interface (USB1) that can
operate in device mode at high-speed. The NBytes value represents the number of bytes
that can be received in the buffer.
* Problem:
If the buffer length is less than the maximum packet size and if the application code does
not program the maximum packet size, the NByte value is not correct.
* Work-around:
Program the NByte to the maximum packet size of that particular endpoint type. The
application code must calculate the received number of bytes by subtracting the NByte
value from the programming value. The software work-around is implemented on the SDK
software platform for the LPC546xx device family.

@hathach
Copy link
Owner

hathach commented Aug 9, 2023

yeah, thank for the info, I should have checked errata. As tested, I also saw it in the bulk in as well (12 instead 13 bytes for msc status) which is more severe as there is no walkaround.

image

A quick look around, and there is tons of errata, tx data corruption 3.22 is also severe

image

I could add walkarond for bulk in/out so that it is kind of usable with stock examples. but I don't know if we have time to go through all of errata, look like usb on lpc54 is a headache one. I would definitely avoid lpc54 for a production with usb.

…B.1 and USB.2

msc is mounted, but device couldn't work reliably and got constant reset
due to other errata probably.

def test_cdc_msc(id):
port = f'/dev/ttyUSB_{id[-8:]}.00'
file = f'/media/blkUSB_{id[-8:]}.02/README.TXT'
Copy link
Owner

Choose a reason for hiding this comment

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

@HiFiPhile why would you limit the serial number to 8 characters, wouldn't be simpler to use it as it is ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For some historical reason my udev script cut serial number to 8 characters.

In fact udev rule is not even needed, cdc can be accessed by /dev/serial/by-id/usb-TinyUSB_TinyUSB_Device_41003B000E504E5457323020-if00

Copy link
Owner

@hathach hathach Aug 9, 2023

Choose a reason for hiding this comment

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

yeah, I figure that out as well. I have already made the changes, just want double check if you first. The id is also shorten in blk device, if it only impact serial, then we are probably safe to remove the cut ?

def get_serial_dev(id, product, ifnum):
    # get usb serial by id
    return f'/dev/serial/by-id/usb-TinyUSB_{product}_{id}-if{ifnum:02d}'


def get_disk_dev(id, lun):
    # get usb disk by id
    return f'/dev/disk/by-id/usb-TinyUSB_Mass_Storage_{id}-0:{lun}'


def get_hid_dev(id, product, event):
    return f'/dev/input/by-id/usb-TinyUSB_{product}_{id}-{event}'

PS: I am trying to get the script also running with my pi4 :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The id is also shorten in blk device, if it only impact serial, then we are probably safe to remove the cut ?

Yes you are right.

Copy link
Owner

Choose a reason for hiding this comment

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

since I am in the middle of this, let me try to do this as well. At worst, we could revert if I messed up :)

@hathach
Copy link
Owner

hathach commented Aug 9, 2023

I pushed my changes which include:

  • add lpc54608 board, similar to 54628
  • work-around for lpc54 mcu, msc disk appears but the mcu is too troublesome, it still reset randomly, and failed with hid_boot_interfaces, probably due to USB.15 (TX corruption) and/or other usb errata. So I decide it is better to not include lpc54 into the test farm since it will be a headache in the future.
  • update hitl_test script and json to simplify the json configuration. by default, all tests will be run. json can specify which specific tests with tests or which to skip with tests_skip.
  • integrate with ci, ci currently compile with cmake + IAR. Which we will use to flash
  • rename hitl to hil, since it seems to be more popular abbreviation

PS: I still need to do more work to update my pi4 with this excellent script. Though pi4 auto mount msc device, but that could be done later once this running well on your VM.

name: stm32l4
path: |
*.elf
python3 test/hil/hil_test.py hil_hfp.json
Copy link
Owner

Choose a reason for hiding this comment

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

this is where the script is run, CI compile with IAR using cmake build in previous step. We could pick up the elf. This will conveniently test IAR for us as well. (normally I only test with gcc)

Copy link
Owner

@hathach hathach left a comment

Choose a reason for hiding this comment

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

everything seems to work well now, except for the lpc54 which has horrible usb hs implementation. The MSC still use the 8 byte ID ( didn't manage time to update that). But it is all good for the merge. We can update thing later with follow-up PRs. @HiFiPhile let me know if you are ok with my changes, and/or want to make more changes.

@hathach
Copy link
Owner

hathach commented Aug 9, 2023

before I forgot here is the lpc54 setting, should we want to re-add it later (couldn't find way to comment out in json)

        {
            "name": "lpcxpresso54608",
            "uid": "0123456789ABCDEF",
            "debugger": "jlink",
            "debugger_sn": "727600775",
            "cpu": "LPC54608J512",
            "tests_skip": [
                "cdc_msc"
            ]
        }

@HiFiPhile
Copy link
Collaborator Author

Perfect it's good for me, maybe also let it run for GCC build.

@hathach
Copy link
Owner

hathach commented Aug 9, 2023

Perfect it's good for me, maybe also let it run for GCC build.

yeah, that is possible as well, though I think we are good enough for now. Since we do generally test with gcc when developement. IAR is much rarer, so I am glad it is covered by HIL. Thank you very much for work and VM/board farm for doing actual test.

@hathach hathach merged commit 04f0cd5 into hathach:master Aug 9, 2023
40 checks passed
@HiFiPhile HiFiPhile deleted the hitl branch April 26, 2024 16:04
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.

2 participants