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

fix system command #1888

Merged
merged 25 commits into from
Jul 24, 2024
Merged

fix system command #1888

merged 25 commits into from
Jul 24, 2024

Conversation

proddy
Copy link
Contributor

@proddy proddy commented Jul 23, 2024

add more tests, all seems to work. I'll keep adding tests for the unknown calls

@proddy proddy requested a review from MichaelDvP July 23, 2024 16:58
@MichaelDvP
Copy link
Contributor

This removes the return_not_found from system, but the others in analogsensor, temperaturesensor, customEntity, scheduler will give the same message if there is a cf found. I still think it's better to clear the output after cf is found,
There still the check:

EMS-ESP32/src/command.cpp

Lines 349 to 353 in dfe85b9

// if we don't alread have a message set, set it to invalid command
if (!output["message"]) {
output["message"] = err;
}
return CommandRet::ERROR;

that keeps the messages from system, analog, temperatur, etc. If you remove all the return_not_found this check is also not needed.
This is safe, much less error-prone and shows all messages from the value_info if there is no cf.
https://github.com/MichaelDvP/EMS-ESP32/blob/bc7d848b9b64c968f451b1a42b531c3547e7a982/src/command.cpp#L356

@proddy
Copy link
Contributor Author

proddy commented Jul 23, 2024

I see your logic. Let me make some changes and add some more tests. I found some missing checks with missing attributes in the sensors too.

@proddy
Copy link
Contributor Author

proddy commented Jul 23, 2024

I added some tests - they all succeed. Please take another look. If I add the root.clear() earlier after a successful cf, it will make unknown commands always return 'info'.

src/console.cpp Outdated Show resolved Hide resolved
@proddy
Copy link
Contributor Author

proddy commented Jul 24, 2024

I'm going to move some checks around too. I still think it's a waste to create device_info and then when there is an invalid entity or command wipe the json output and replace with an error. It's expensive and a waste. I'll move the deviceID check earlier in the code and do some more cleaning.

@MichaelDvP
Copy link
Contributor

I still think it's a waste to create device_info and then when there is an invalid entity or command wipe the json output and replace with an error.

That's mainly the return_not_found you have placed at the end of each get_value_info and then check it before checking the cf functions. It's always the same output that has to be cleared and can be replaced by single output in the if (!cf) section. The attiributs error can stay, it will never conflict with cf.

@proddy
Copy link
Contributor Author

proddy commented Jul 24, 2024

attempt #4 - think it works, at least the tests pass. And I didn't need to flush root.clear()

src/command.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@MichaelDvP MichaelDvP left a comment

Choose a reason for hiding this comment

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

Yes, looks all ok now,
Only thing is that i prefere log_info for the commands with values.

@proddy proddy merged commit 1b666a0 into emsesp:dev Jul 24, 2024
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