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

Add shell management #4

Merged
merged 4 commits into from
Feb 13, 2024
Merged

Conversation

petrovichkr
Copy link
Contributor

Add shell management group based on Zephyr Docs.

@JPHutchins
Copy link
Owner

@petrovichkr Hi, thanks so much for this contribution!

I have been slow to add docs here and now it is slowing you down 😢

To lint:

. ./envr.ps1
lint

Or run without the envr alias:

black --check . && isort --check-only . && flake8 . && mypy .

Copy link
Owner

@JPHutchins JPHutchins left a comment

Choose a reason for hiding this comment

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

Great! I will create a ticket to improve the _do_test function. Now that I'm looking at my own _do_test code again it looks too "magic" to me and I want it to be easier for contributors. I'd love your advice!

In order to test these with a device, I'll bump smpclient and smpmgr to implement the shell management group that you've added. Very useful (and dangerous), especially over BLE!

Comment on lines 49 to 55
def test_ExecuteRequest() -> None:
_do_test(
smpshell.ExecuteRequest,
smphdr.OP.WRITE,
shellcmd.EXECUTE,
{"argv": ["echo", "Hello"]},
)
Copy link
Owner

Choose a reason for hiding this comment

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

Let's update to:

def test_ExecuteRequest() -> None:
    r = _do_test(
        smpshell.ExecuteRequest,
        smphdr.OP.WRITE,
        shellcmd.EXECUTE,
        {"argv": ["echo", "Hello"]},
    )
    assert r.argv == ["echo", "Hello"]

so that anyone looking at the tests can see usage a bit more clearly.

That _do_test function I made looks confusing and I'll create an issue to make it better and prevent us from copying it for every group of tests.

Comment on lines 58 to 64
def test_ExecuteResponse() -> None:
_do_test(
smpshell.ExecuteResponse,
smphdr.OP.WRITE_RSP,
shellcmd.EXECUTE,
{"o": "Hello", "ret": 0},
)
Copy link
Owner

Choose a reason for hiding this comment

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

Same idea:

def test_ExecuteResponse() -> None:
    r = _do_test(
        smpshell.ExecuteResponse,
        smphdr.OP.WRITE_RSP,
        shellcmd.EXECUTE,
        {"o": "Hello", "ret": 0},
    )
    assert r.o == "Hello"
    assert r.ret == 0

@petrovichkr
Copy link
Contributor Author

@petrovichkr Hi, thanks so much for this contribution!

I have been slow to add docs here and now it is slowing you down 😢

To lint:

. ./envr.ps1
lint

Or run without the envr alias:

black --check . && isort --check-only . && flake8 . && mypy .

Done fixing lint issues. This is my first contribution to the Python project, so sorry too :)

@petrovichkr
Copy link
Contributor Author

Great! I will create a ticket to improve the _do_test function. Now that I'm looking at my own _do_test code again it looks too "magic" to me and I want it to be easier for contributors. I'd love your advice!

My test writing skills are about zero, so I'm a bad advisor :) I created these tests based on your test_os_management.

In order to test these with a device, I'll bump smpclient and smpmgr to implement the shell management group that you've added. Very useful (and dangerous), especially over BLE!

Overall these tools will be a good replacement for the existing newtmgr/mcumgr. BLE transport is much more stable and doesn't require root rights. I'm going to use the smpclient package to develop automation utilities for testing Zephyr based hardware over BLE.

By the way, I've already implemented the shell group in smpclient. I can do PR to smpclient after the release of this project.

@JPHutchins
Copy link
Owner

Overall these tools will be a good replacement for the existing newtmgr/mcumgr.

That's the goal, but there's a lot of work and testing to be done! While there are still only a few people using these tools, breaking changes may be frequent to arrive at a consensus API. You can trust the semantic versioning and release notes to indicate these breaking changes.

@JPHutchins JPHutchins self-requested a review February 13, 2024 18:57
Copy link
Owner

@JPHutchins JPHutchins left a comment

Choose a reason for hiding this comment

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

Thanks!

@JPHutchins JPHutchins merged commit 3c6c675 into JPHutchins:main Feb 13, 2024
4 checks passed
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