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 machine readable serial formatting #233

Merged

Conversation

geekuillaume
Copy link
Contributor

This PR introduces a new verbose mode "machine_readable" designed to work with https://github.com/geekuillaume/simplefoc-webcontroller.

It's very similar to existing verbose mode with some changes being made to always echo back the command received by the controller + the value that was set (or just print the existing value if no value was set). This makes the communication with the controller way more reliable.

@runger1101001
Copy link
Member

Supernice, I love the Web-Commander, I can't wait to try it out!

I wonder if it isn't better to make a dedicated class for it, rather than trying to roll it into the existing Commander class?

Please note that we only accept PRs against the "dev" branch, not "master". I'll change it, but you may want to update your local git repo to use the dev branch...

@runger1101001 runger1101001 changed the base branch from master to dev December 5, 2022 14:03
@geekuillaume
Copy link
Contributor Author

I'm not sure what's the best way to handle this. There are some code path that are not as clean as they could be but changing them would mean refactoring a lot of other parts of the code that are not related to the "machine readable" format. I tried to keep my changes to the minimum in this PR. I'll maybe work on another code architecture for this in a future PR.

@askuric askuric changed the title Add machine readable serial formating Add machine readable serial formatting Dec 20, 2022
@askuric
Copy link
Member

askuric commented Dec 22, 2022

Hey guys,

@geekuillaume thanks again for the contribution. I've posted a few question in the code so when you have time please do let me know what you think.
I've recently extended (not yet in dev) the monitoring api a bit with variable start and end character, separator and the decimal places. So its very much in line with what you have done. :D

  • At the moment this PR is not compiling due to not having motor_prepend_id defined in the .h file. But don't worry about that because the nomenclature and the monitoring code will probably be a bit different.
  • Regarding the commander, the only issue that I have is regarding the monitoring again. Is there a reason behind the extension of the monitoring variable setting to 7 variables. I am not sure I understand that one.

Thanks again for your time,
Antun

@askuric askuric merged commit 02d7b47 into simplefoc:dev Mar 10, 2023
@askuric
Copy link
Member

askuric commented Mar 10, 2023

Hey @geekuillaume,
I've merged your changes to the dev branch and they will be a part of the next release. The changes to the commander have been taken as is, everything except the 8 variable monitoring which was I suppose a typo in your code.

In the FOCMotor I've opted for a different and a bit more flexible solution for adding characters in the monitor() function. We have a new api for monitoring that will enable setting the start and end character as well as the separation character and the number of decimal places. With all of this in mind, I did not opt for your changes.
So instead of adding two characters at the begging of the monitoring output one being motor id and the other the letter M, I'd suggest to use only the motor id as the start and the end character of the monitoring line.

In our fork of your applicaiton I've implemented that style of parsing the monitoring output. Here's the link:
https://github.com/simplefoc/simplefoc-webcontroller/tree/gh-pages

I'll integrate your application in the docs.simplefoc.com for the next release.
Please let me know if you want me to PR the changes that I've made.

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