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

AAVAA Board Changes and \x00 config support #684

Closed

Conversation

aavaa-farnood
Copy link
Contributor

I added some new changes:

  • AAVAA Board new channels and data diagram
  • Changed the config_board method in a way that it accepts \x00 bytes as well by passing the config length to the cpp method.

@Andrey1994
Copy link
Member

Hi, need to fix failed CI jobs before merge

@Andrey1994 Andrey1994 self-requested a review November 5, 2023 23:20
Copy link
Member

@Andrey1994 Andrey1994 left a comment

Choose a reason for hiding this comment

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

It breaks config_board method for all languages other than cpp and python.
Change in method signature should be made also for java, C#, julia, Rust, Js and matlab.

CI is green because in tests we mostly ignore config_board since its hard to emulate proper response

@Andrey1994
Copy link
Member

also, not really sure, that its a good idea to send \0 directly via config_board method and add support for it. Its converted back and forth to\from plain C string in random places for different boards, its not good and as a result it's pretty simple to miss smth and get a mismatch between strlen and original size.

If you need to send such byte for your device maybe it makes sense to introduce human readable commands and convert them to actual commands sent to device(like you did)

@aavaa-farnood
Copy link
Contributor Author

aavaa-farnood commented Nov 6, 2023

Oh I see, I just checked to see if it passes all the CI/CD workflows and thought we're good to go.
I can also change it in other languages.

Regarding the \x00 byte, we need it since we need to send floats to the board and it's not really efficient to send it in a human readable format since we need different precisions for different use-cases. Do you have any other suggestions for this use-case?

@Andrey1994
Copy link
Member

Regarding the \x00 byte, we need it since we need to send floats to the board and it's not really efficient to send it in a human readable format since we need different precisions for different use-cases. Do you have any other suggestions for this use-case?

But according to convention its expected that this method gets a string from the user and its not designed to handle bytes directly.

I would highly recommend to implement higher level commands in config_board method. For example smth like this in user api

board.config_board("%meaningful name for your command here%")
// you can even implement smth like json commands here or design your own format for particular device e.g.
board.config_board("enable_channel: 1")

This way its more user friendly, since customers dont need to operate with bytes

And inside config board method implementation for aavva device you can write some if else logic to convert this input json/command to machine readable commands

pseudocode:

if (conf == "%command name%)
{
    socket.send(0)
}

@Andrey1994
Copy link
Member

Maybe there is a use case to send bytes directly but I would rather do it as a different method instead patching config_board.
And before implementing it as an another methods will need to understand if its really useful or not, for now I would recommend to implement high level commands as string

@Andrey1994
Copy link
Member

Any updates on this?

@Andrey1994
Copy link
Member

there is an option to send bytes now, if still needed feel free to create another PR, closing this one since there is no activiti

@Andrey1994 Andrey1994 closed this Feb 19, 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.

3 participants