search

openjournals / joss-reviews

Reviews for the Journal of Open Source Software
Creative Commons Zero v1.0 Universal
725 stars 38 forks source link

[REVIEW]: SimpleFOC: A Field Oriented Control (FOC) Library for Controlling Brushless Direct Current (BLDC) and Stepper Motors #4232

Closed editorialbot closed 2 years ago

editorialbot commented 2 years ago

Submitting author: !--author-handle-->@askuric<!--end-author-handle-- (Antun Skuric) Repository: https://github.com/simplefoc/Arduino-FOC/ Branch with paper.md (empty if default branch): joss_paper Version: v2.2.2 Editor: !--editor-->@gkthiruvathukal<!--end-editor-- Reviewers: @sea-bass, @ixjlyons Archive: 10.5281/zenodo.6510536

Status

status

Status badge code:

HTML: <a href="https://joss.theoj.org/papers/4382445f249e064e9f0a7f6c1bb06b1d"><img src="https://joss.theoj.org/papers/4382445f249e064e9f0a7f6c1bb06b1d/status.svg"></a>
Markdown: [![status](https://joss.theoj.org/papers/4382445f249e064e9f0a7f6c1bb06b1d/status.svg)](https://joss.theoj.org/papers/4382445f249e064e9f0a7f6c1bb06b1d)

Reviewers and authors:

Please avoid lengthy details of difficulties in the review thread. Instead, please create a new issue in the target repository and link to those issues (especially acceptance-blockers) by leaving comments in the review thread below. (For completists: if the target issue tracker is also on GitHub, linking the review thread in the issue or vice versa will create corresponding breadcrumb trails in the link target.)

Reviewer instructions & questions

@sea-bass & @ixjlyons, your review will be checklist based. Each of you will have a separate checklist that you should update when carrying out your review. First of all you need to run this command in a separate comment to create the checklist:

@editorialbot generate my checklist

The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @gkthiruvathukal know.

✨ Please start on your review when you are able, and be sure to complete your review in the next six weeks, at the very latest ✨

Checklists

📝 Checklist for @sea-bass

📝 Checklist for @ixjlyons

editorialbot commented 2 years ago

Hello humans, I'm @editorialbot, a robot that can help you with some common editorial tasks.

:warning: JOSS reduced service mode :warning:

Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post.

For a list of things I can do to help you, just type:

@editorialbot commands

For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:

@editorialbot generate pdf
editorialbot commented 2 years ago

Checking the BibTeX entries failed with the following error: 

No paper file path
editorialbot commented 2 years ago 
Software report:

github.com/AlDanial/cloc v 1.88  T=0.13 s (1218.7 files/s, 146488.4 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
C++                             53           1460           2383           6275
Arduino Sketch                  66           1164           2099           2602
C/C++ Header                    37            463           1302           1106
Markdown                         3             80              0            228
YAML                             1             13              1             56
-------------------------------------------------------------------------------
SUM:                           160           3180           5785          10267
-------------------------------------------------------------------------------

gitinspector failed to run statistical information for the repository
editorialbot commented 2 years ago

Failed to discover a Statement of need section in paper

editorialbot commented 2 years ago

:warning: An error happened when generating the pdf.

gkthiruvathukal commented 2 years ago

@sea-bass and @ixjlyons, Have you been able to get started with review? Please follow up here.

sea-bass commented 2 years ago

Review checklist for @sea-bass

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

sea-bass commented 2 years ago

@sea-bass and @ixjlyons, Have you been able to get started with review? Please follow up here.

Hi @gkthiruvathukal -- I have just gone through the software for my review: https://github.com/simplefoc/Arduino-FOC/issues/174

While the software, documentation, etc. looks great, we are still unable to generate the software paper and therefore review that piece. Could you and the author take a look at this and update us when the paper is available?

Additionally, the CI pipeline includes compilation tests, but there are no unit tests, so I have recommended the addition of those for some of the core C++ source.

askuric commented 2 years ago

Hey @sea-bass, Thanks for the review! The paper is the joss_paper branch link. That was one of the suggested ways by the joss webiste ( to add a branch with the paper).

If you prefer I can add the paper to the master. Antun

sea-bass commented 2 years ago

@editorialbot generate pdf

sea-bass commented 2 years ago

Hey @sea-bass, Thanks for the review! The paper is the joss_paper branch link. That was one of the suggested ways by the joss webiste ( to add a branch with the paper).

If you prefer I can add the paper to the master. Antun

Thanks, @askuric -- turns out the main issue description didn't specify a branch, so it defaulted. I've just edited it to the joss_paper branch. Let's see how this turns out.

EDIT: Yep, that worked. I've updated my review and the comment about unit tests is the only major one amidst minor comments.

editorialbot commented 2 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

gkthiruvathukal commented 2 years ago

@sea-bass and @askuric. Thanks for the updates here. @ixjlyons Have you been able to get started with the review?

ixjlyons commented 2 years ago

Review checklist for @ixjlyons

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

ixjlyons commented 2 years ago

@gkthiruvathukal (et al.) apologies for the delayed start - I should be able to finish reviewing in the next couple days.

ixjlyons commented 2 years ago

My review:

SimpleFOC is a library offering a modular approach to implementing field oriented control with various hardware components. I'm currently not able to directly verify functionality of the library, but I've been able to successfully compile several examples and have reviewed portions of the code. The code itself, along with the very well-done documentation, appear to be a great resource for implementing advanced motor control for a variety of applications. I recommend accepting.

General Comments

I may have missed it, but I haven't come across clear community guidelines as required by JOSS in the documentation or README. I think this is the only blocking issue. Aside from that, I only have a few minor comments:

Paper

No major concerns with the paper. Below are some minor comments:

askuric commented 2 years ago

Hey @ixjlyons and @sea-bass, Thank you very much for your insightful and supporting reviews!

@sea-bass With the new release of the library we've spent some time on the gifs/moving images and made them a lot more clear. Thank you for the suggestion. We'll also consider adding more functionality unitary testing, but for now as the code is intended to be run on different MCU architectures and interface with different hardware components the full scale testing is a bit hard to do. But as you well pointed out there are ways to do it and we should do more in this regard. :D

@ixjlyons We've updated the paper to account for your remarks. In the docs, the button has been fixed and we've added much more examples which can be interchanged with a click of the button to make it easier to understand.

Regarding the community guidelines. As many of our users are not really git users we have opted to the scheme where they are free to propose and share their problems/contributions within the community framework And if they are motivated we help them to create the PR request and we really treat each one case by case. But you are absolutely right that we would need a bit more structures issue template and PR template in order to make contribution and issue reporting more efficient and more clear. We add the guidelines to the github repo this till the end of this week.

Thanks again for your time and let me know if there is something else that you'd like us to consider!

ixjlyons commented 2 years ago

@ixjlyons We've updated the paper to account for your remarks. In the docs, the button has been fixed and we've added much more examples which can be interchanged with a click of the button to make it easier to understand.

Looks good to me :+1:

Regarding the community guidelines. As many of our users are not really git users we have opted to the scheme where they are free to propose and share their problems/contributions within the community framework And if they are motivated we help them to create the PR request and we really treat each one case by case.

I think that's fine -- my understanding is there just needs to be guidelines written somewhere to make expectations clear. I'd say the links in the header of the docs cover getting help and maybe even bringing up issues. "Please open an issue to propose improvements" in the README would be sufficient I think. Pull request / issue templates are great additions too.

sea-bass commented 2 years ago

@sea-bass With the new release of the library we've spent some time on the gifs/moving images and made them a lot more clear. Thank you for the suggestion. We'll also consider adding more functionality unitary testing, but for now as the code is intended to be run on different MCU architectures and interface with different hardware components the full scale testing is a bit hard to do. But as you well pointed out there are ways to do it and we should do more in this regard. :D

Of course! I was only suggesting unit testing, as having full-scale (or system-level) testing for a hardware-focused library would be unreasonable.

askuric commented 2 years ago

Hey @ixjlyons, Ok so we've added a short contributing webpage as the addition to the contact page in the docs.

Also the README was extended with the section about contributing and different community platforms that are available.

Finally, I've added the first version of issue templates that should enable users to post more direct and precise questions/issues. For the PRs it is a bit more tricky so we will provide the templates a bit later. In any case, the docs and the README and our communications are clearly oriented in a way to encourage the users to contribute in any way they feel most conformable about.

ixjlyons commented 2 years ago

Great, looks good to me. Unless there's still a hangup on testing, I think that was the last outstanding issue. So this should be ready to go, nice work.

sea-bass commented 2 years ago

According to the guidelines, the requirements are:

Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?

If there is no plan to create automated unit tests / a CI pipeline for this review process (which is a totally fair decision), could you point me to the "manual steps" that would meet the criteria above? And if not available, could some documentation on testing this software and verifying it works as intended be made?

Thanks!

askuric commented 2 years ago

Hey everyone,

I am sorry for a late reply!

At the moment the CI pipeline for unit testing of the software components is really not our priority. Mostly because the most of the problems due to the library code comes from the MCU specific issues which cannot really be unit tested. We are definitely interested in integrating the unit testing at some point (the sooner the better) but at the moment we do not have the time resources to do it :D We are however making sure that the library compiles for each one of the supported platforms using the githubs CIs, but for the moment not more than that.

However, we do have guidelines in the docs that explain how each user can verify that each part of the library works correctly with their hardware. This is a very important step for setting up any of the users hardware setups, regardless of how standard or nonstandard they are. So in an way each user really needs to test and verify that the library code works well with their hardware. Additionally we support many different debugging and monitoring features which facilitate testing as well. Getting started explains step by step testing/verification procedure: docs page. Additionally there are many examples codes in the library that are intended for testing and verifying that library works as expected with given hardware. Library examples link

As basically all of our users use our code only in combination with hardware components, till this point we did not consider putting in place any purely software testing procedures, all of our testing procedures are coupled with hardware components.

sea-bass commented 2 years ago

@askuric thank you for clarifying that and pointing me to the right resources -- I think that absolutely meets the testing requirements, and agree with your prioritization especially given the hardware-focused nature of the project.

LGTM :)

askuric commented 2 years ago

Thanks @ixjlyons and @sea-bass for yout reviews, we appreciate it! @gkthiruvathukal don't hesitate to let us know what we need to do to advance to the next step.

I have one question though, would it be possible to chnage the title of the paper to: "SimpleFOC: A Cross-Platform Field Oriented Control (FOC) Library for Controlling Brushless Direct Current (BLDC) and Stepper Motors"

I think it might be important to add 'Cross-Platform' to the title as one of out main contributions.

gkthiruvathukal commented 2 years ago

@askuric It looks like we are good to go. I will follow up with the next steps momentarily!

gkthiruvathukal commented 2 years ago

@editorialbot generate pdf

editorialbot commented 2 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

gkthiruvathukal commented 2 years ago

@askuric: Please do the following:

askuric commented 2 years ago

Hello @gkthiruvathukal,

  1. We already have a zenodo entry for our releases so I've updated the metadata of the paper for our latest release. I hope that is fine with you.

    This release does not have paper source inside, let me know if we are required integrate it into our release.

  2. I've changed the title of the paper just a bit, by adding "Cross-Platform" to the title "SimpleFOC: A Cross-Platform Field Oriented Control (FOC) Library for Controlling Brushless Direct Current (BLDC) and Stepper Motors" I hope that you are fine with it, as it is one of our main contributions. It is updated in paper source in the the joss_paper branch and in the Zenodo entry.

    If the title change is not possible I'll revert to the original version as soon as possible.

So our software release version is: v2.2.2

The Zenodo DOI value: 10.5281/zenodo.6510536

gkthiruvathukal commented 2 years ago

@askuric I am ok with the proposed title change. However, I am also going to ask the @openjournals/joss-eics for input, as I don't know whether we have policies about changing the title of a submission. The proposed change seems helpful to me.

askuric commented 2 years ago

@gkthiruvathukal thanks. If you think this will take too long to validate, we can aslo proceed with the original title. I can change it right away.

gkthiruvathukal commented 2 years ago

@askuric I prefer to stick with the original title. Once you have it back to the original, can you let me know, so I an finalize acceptance?

askuric commented 2 years ago

OK, I understand, it's done.

So our software release version is: v2.2.2

The Zenodo DOI value: 10.5281/zenodo.6510536

gkthiruvathukal commented 2 years ago

@editorialbot set 10.5281/zenodo.6510536 as archive

editorialbot commented 2 years ago

Done! Archive is now 10.5281/zenodo.6510536

gkthiruvathukal commented 2 years ago

@editorialbot set v2.2.2 as version

editorialbot commented 2 years ago

Done! version is now v2.2.2

gkthiruvathukal commented 2 years ago

@editorialbot generate pdf

editorialbot commented 2 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

gkthiruvathukal commented 2 years ago

@editorialbot recommend-accept

editorialbot commented 2 years ago 
Attempting dry run of processing paper acceptance...
editorialbot commented 2 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1109/CoDIT49905.2020.9263910 is OK
- 10.1109/EIConRus.2018.8317164 is OK
- 10.1049/iet-pel.2018.5231 is OK
- 10.1109/ICRERA.2018.8566749 is OK
- 10.21105/joss.00456 is OK
- 10.46842/ipn.cien.v25n1a05 is OK
- 10.1109/IPRECON49514.2020.9315210 is OK
- 10.4230/OASIcs.PARMA-DITAM.2021.3 is OK
- 10.1109/IECON.2016.7793092 is OK
- 10.1109/IEIT53149.2021.9587419 is OK
- 10.1109/IROS.2012.6386252 is OK

MISSING DOIs

- 10.51257/o-42509210 may be a valid DOI for title: Mécatronique

INVALID DOIs

- None
gkthiruvathukal commented 2 years ago

@askuric Please check the MISSING DOIs. Thanks!

editorialbot commented 2 years ago

:wave: @openjournals/joss-eics, this paper is ready to be accepted and published.

Check final proof :point_right: https://github.com/openjournals/joss-papers/pull/3297

If the paper PDF and the deposit XML files look good in https://github.com/openjournals/joss-papers/pull/3297, then you can now move forward with accepting the submission by compiling again with the command @editorialbot accept

askuric commented 2 years ago

@gkthiruvathukal thanks! The book Méchatronique does not have a DOI number, the suggested DOI is not referencing the same book. In the paper we have provided the ISBN number of the book and the year of publishing.

Kevin-Mattheus-Moerman commented 2 years ago

@askuric

askuric commented 2 years ago

Hi @Kevin-Mattheus-Moerman,

Thanks for the updates. The countries are added to the affiliations, don't hesitate to let me know if there are other changes necessary.

Kevin-Mattheus-Moerman commented 2 years ago

@editorialbot generate pdf

editorialbot commented 2 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left: