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

Rename folders for clarity [AEK-269] #36

Merged
merged 7 commits into from Dec 30, 2022

Conversation

aliphys
Copy link
Member

@aliphys aliphys commented Jul 23, 2021

No description provided.

@aliphys aliphys changed the title Rename folders fro clarity [AEK-269] Rename folders for clarity [AEK-269] Jul 23, 2021
Copy link
Contributor

@per1234 per1234 left a comment

Choose a reason for hiding this comment

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

Please resolve the sketch name mismatch at examples/Firmware/Flasher.ino.

Copy link
Contributor

@per1234 per1234 left a comment

Choose a reason for hiding this comment

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

The examples paths in the "Compile Examples" CI workflow must be updated at the same time these examples are moved:

@CLAassistant
Copy link

CLAassistant commented Jul 28, 2021

CLA assistant check
All committers have signed the CLA.

@aliphys aliphys requested a review from per1234 July 28, 2021 12:47
@@ -46,7 +46,7 @@ void setup() {
// This way, you can program the motor to reach a certain position or velocity without any further intervention.
// The PID can be carefully tuned if a particular profile is needed.
pid1.setControlMode(CL_POSITION);
pid1.setGains(25, 0, 3);
pid1.setGains(25.0f, 0.0f, 3.0f);
Copy link
Member Author

Choose a reason for hiding this comment

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

pid1.setGains() works with float as the input datatype (not int). Thanks @Rocketct 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

@per1234 Test.ino should compile correctly now 👍

@github-actions
Copy link

Memory usage change @ cb2af4b

Board flash % RAM for global variables %
arduino:samd:mkr1000 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
arduino:samd:mkrfox1200 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
arduino:samd:mkrgsm1400 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
arduino:samd:mkrnb1500 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
arduino:samd:mkrvidor4000 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
arduino:samd:mkrwan1300 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
arduino:samd:mkrwan1310 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
arduino:samd:mkrwifi1010 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
arduino:samd:mkrzero 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
Click for full report table
Board examples/Flasher
flash
% examples/Flasher
RAM for global variables
% examples/MKRMotorCarrier/Motor_test
flash
% examples/MKRMotorCarrier/Motor_test
RAM for global variables
% examples/MKRMotorCarrier/Motor_test_encoder
flash
% examples/MKRMotorCarrier/Motor_test_encoder
RAM for global variables
% examples/MKRMotorCarrier/Servo_test
flash
% examples/MKRMotorCarrier/Servo_test
RAM for global variables
% examples/MKRMotorCarrier/Test
flash
% examples/MKRMotorCarrier/Test
RAM for global variables
%
arduino:samd:mkr1000 0 0.0 0 0.0 N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A
arduino:samd:mkrfox1200 0 0.0 0 0.0 N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A
arduino:samd:mkrgsm1400 0 0.0 0 0.0 N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A
arduino:samd:mkrnb1500 0 0.0 0 0.0 N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A
arduino:samd:mkrvidor4000 0 0.0 0 0.0 N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A
arduino:samd:mkrwan1300 0 0.0 0 0.0 N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A
arduino:samd:mkrwan1310 0 0.0 0 0.0 N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A
arduino:samd:mkrwifi1010 0 0.0 0 0.0 N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A
arduino:samd:mkrzero 0 0.0 0 0.0 N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A N/A
Click for full report CSV
Board,examples/Flasher<br>flash,%,examples/Flasher<br>RAM for global variables,%,examples/MKRMotorCarrier/Motor_test<br>flash,%,examples/MKRMotorCarrier/Motor_test<br>RAM for global variables,%,examples/MKRMotorCarrier/Motor_test_encoder<br>flash,%,examples/MKRMotorCarrier/Motor_test_encoder<br>RAM for global variables,%,examples/MKRMotorCarrier/Servo_test<br>flash,%,examples/MKRMotorCarrier/Servo_test<br>RAM for global variables,%,examples/MKRMotorCarrier/Test<br>flash,%,examples/MKRMotorCarrier/Test<br>RAM for global variables,%
arduino:samd:mkr1000,0,0.0,0,0.0,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A
arduino:samd:mkrfox1200,0,0.0,0,0.0,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A
arduino:samd:mkrgsm1400,0,0.0,0,0.0,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A
arduino:samd:mkrnb1500,0,0.0,0,0.0,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A
arduino:samd:mkrvidor4000,0,0.0,0,0.0,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A
arduino:samd:mkrwan1300,0,0.0,0,0.0,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A
arduino:samd:mkrwan1310,0,0.0,0,0.0,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A
arduino:samd:mkrwifi1010,0,0.0,0,0.0,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A
arduino:samd:mkrzero,0,0.0,0,0.0,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A

@Rocketct Rocketct self-requested a review August 2, 2021 07:46
Copy link
Contributor

@Rocketct Rocketct left a comment

Choose a reason for hiding this comment

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

you added examples/NanoMotorCarrier and examples/MKRMotorCarrier but if you don't change the folder name too on the file system, the github action will fail when the will try to find these path

@aliphys
Copy link
Member Author

aliphys commented Aug 2, 2021

@Rocketct can you please help me change the required files required for the GitHub Action? I am learning GitHub actions as we go along. 😃

@Rocketct
Copy link
Contributor

Rocketct commented Aug 2, 2021

https://github.com/arduino-libraries/ArduinoMotorCarrier/tree/master/examples check the names here as you can see here the folder name are MKR and Nano, in your PR you changed the name in the github action as examples/NanoMotorCarrier and examples/MKRMotorCarrier you can do two action:

  • modify the folder name and use the new naming
  • get back the name to MKR and Nano

one think @aliphys , did you discused these changes with @ErnestoELC ?
i don't know if this could be a problem with matlab file system management

@aliphys
Copy link
Member Author

aliphys commented Aug 2, 2021

@Rocketct I have made the changes you mention in commit 5dd1e20 .
Good question about the MATLAB integration, I will ask in our stand-up tomorrow. 👍

@Rocketct
Copy link
Contributor

Rocketct commented Aug 2, 2021

@aliphys This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.

please double check apparently that commit was not merged, or is not present on master, make that changes and add it to this PR.

but first of all talk with @ErnestoELC this repository cannot be modified so lightly as the others, second and important point because we are using an implicit cast can you and @ErnestoELC check if works or not, because could happen stranger things so we shall be sure that is working properly.

@aliphys
Copy link
Member Author

aliphys commented Aug 3, 2021

@Rocketct just spoke with MW, changing the examples folder and its content does not influence the Hardware Support Packages.

@Rocketct
Copy link
Contributor

Rocketct commented Aug 3, 2021

okk perfect so make the changes on the folders name and after the test could be merged

@Rocketct
Copy link
Contributor

Rocketct commented Aug 3, 2021

ah okk looks like you have made it let me check if the github action pass

@Rocketct
Copy link
Contributor

Rocketct commented Aug 3, 2021

@per1234 ciao Per can you check the github action? it fails when start to compile the examples

@per1234
Copy link
Contributor

per1234 commented Aug 3, 2021

@Rocketct everything looks as expected to me. There are some compilations that fail, but those are true results.

You will see some of these if you look in the logs:

Compiling sketch: examples/MKRMotorCarrier/Motor_test_encoder
Compiling previous version of sketch to determine memory usage change
Compiling sketch: examples/MKRMotorCarrier/Motor_test_encoder
  
  Error during build: opening sketch: no valid sketch found in /home/runner/work/ArduinoMotorCarrier/ArduinoMotorCarrier/examples/MKRMotorCarrier: missing /home/runner/work/ArduinoMotorCarrier/ArduinoMotorCarrier/examples/MKRMotorCarrier/MKRMotorCarrier.ino
  
Error: Compilation failed
Change in flash: N/A
Change in RAM for global variables: N/A

The failure of the "Compiling previous version of sketch to determine memory usage change" compilation is normal and expected in this case where the sketch did not exist at that path in the PR's base ref. This second compilation is only done for the sake of the memory usage change report, which is purely informational. The failure of that compilation does not change the CI result. It will start working fine on the next PR after the sketches have been established at their new location.

So focus only on the parts of the logs where the first compilation failed. Those are the ones that cause the ❌ .


Note also that there were widespread sporadic GitHub Actions outages all day. My email inbox is full of notifications of spurious workflow failures. These were transient so they were usually passing after I re-ran the jobs. The results you see from this PR are from days ago, before that happened, but if you tried running the CI today then you might have encountered some of these failures. GitHub Actions seems to be increasingly unreliable lately, but at least they get it back up to normal fairly quickly.

@Rocketct
Copy link
Contributor

Rocketct commented Aug 4, 2021

@per1234 ah ok i though that these ones:

Compiling sketch: examples/MKRMotorCarrier/Motor_test_encoder
Compiling previous version of sketch to determine memory usage change
Compiling sketch: examples/MKRMotorCarrier/Motor_test_encoder
  
  Error during build: opening sketch: no valid sketch found in /home/runner/work/ArduinoMotorCarrier/ArduinoMotorCarrier/examples/MKRMotorCarrier: missing /home/runner/work/ArduinoMotorCarrier/ArduinoMotorCarrier/examples/MKRMotorCarrier/MKRMotorCarrier.ino
  
Error: Compilation failed
Change in flash: N/A
Change in RAM for global variables: N/A

was fails, so fine i think that we should focus on portenta's one, but i think that there is a mistake because as far as i remember this library should not be compatible with portenta, can you confirm @aliphys @ErnestoELC ?

@aliphys
Copy link
Member Author

aliphys commented Aug 4, 2021

@per1234 I appreciate your explanation of the GH actions. 👍

@Rocketct Neither boards are compatible with the Portenta, with no plan to provide compatibility in the near future. The Motor Carrier library is designed to work with the Nano 33 IoT (for the Nano Motor Carrier examples) and all the MKR boards (for the MKR Motor Carrier). You can see the list of compatible boards on their relevant docs pages.

Note: Nano BLE 33 Sense compatibility is experimental at the moment.

@per1234
Copy link
Contributor

per1234 commented Aug 4, 2021

I'm the one who added the compilation for Portenta H7 to the CI workflow. The reason is because this statement:
https://store.arduino.cc/portenta-h7

MKR Headers | Use any of the existing industrial MKR shields on it

implied to me that the MKR form factor accessories are intended to be compatible with the Portenta. If the hardware is intended to be compatible then I would expect the software to be compatible as well.

@Rocketct
Copy link
Contributor

Rocketct commented Aug 4, 2021

@per1234 ah ok mmm for this particular case should not or better was not implemented the software support to this shield and i don't think we want it, but if mandatory we could see how to implement it

@per1234
Copy link
Contributor

per1234 commented Aug 4, 2021

Well, I had to make some educated guesses as to which boards a library was intended to support when it came to setting up the compilation workflows. The point of the workflow is to find example/board combos that don't compile, so I couldn't use a failure to compile as an indication that a board should not be compiled for.

In cases where the CI is configured to compile for a board when there is no intention of the library supporting that board, I'm always happy to correct the CI configuration. However, in that case I would also suggest the evaluation of whether the supported boards can be more clearly communicated to the user, since if it was unclear to me then it might well be unclear to others.

@Rocketct
Copy link
Contributor

Rocketct commented Aug 4, 2021

the portenta and the shield are hw incompatible

@per1234
Copy link
Contributor

per1234 commented Aug 4, 2021

PR submitted to remove the Portenta compilations from the CI workflow: #39

@aliphys
Copy link
Member Author

aliphys commented Aug 4, 2021

the portenta and the shield are hw incompatible

@Rocketct can you kindly clarify why they are HW incompatible? The Portenta does have all the pins for the MKR boards.

@Rocketct
Copy link
Contributor

Rocketct commented Aug 4, 2021

A1 and A2 cannot be used as analog out and they are used to controls motors through the shield, so the shield and the Portenta cannot be used together.
So there is no reason to fix the software because at last the hardware will not works than have no sense include it on the .github action because we don't want to check something that is no compatible and will never be changed

@aliphys
Copy link
Member Author

aliphys commented Aug 4, 2021

@Rocketct I have taken a look at the schematic. A1 and A2 pass through the bidirection voltage translator and then connect to IN2 and IN4 respectively. How does the lack of PWM influence the ADC capacilities?

@Rocketct
Copy link
Contributor

Rocketct commented Aug 4, 2021

are two different things, however in this case the problem is that from A0 to A4 are connected to a switch (analog switch) that allows to choose if use this pin as PXY_C or PXY, really the incompatibility born in the exactly moment when you decide to use the SPI, that i notice is not populated so there could be a chance to make it usable, but in general if you use alone the pins only as a pure analog there shouldn't be any problem, but if you use PXY and meanwhile you want to use PXY_C functionality this is not possible, because from an hw point of view you are allowed to use only one pad at time.

@aliphys
Copy link
Member Author

aliphys commented Aug 5, 2021

@Rocketct If we use the labelled pins only for analog input (i.e. use the ADC), then there shouldn't be a HW blocker for using the Portenta with the MKR Motor Carrier, is this correct?
image

@Rocketct
Copy link
Contributor

Rocketct commented Aug 5, 2021

no @aliphys that pin is conneted to sig INX pin, but AFAIK that connector is used to connect motors too rigth? if yes the sig signal shall send some signal to control the motor than analog out.
But looks the SPI have DNP so this means that the SPI is disconnected than this means that this could not affect the compatibility, than the product could works with portenta because you can trhough AnalogWrite/analogread switch the analog witch than use the corect pad configuration and use A1/A2 as output.

@aliphys
Copy link
Member Author

aliphys commented Aug 5, 2021

The INX (IN1-IN4) connectors are designated as analog inputs (so the user can attach external sensors) and not used to control the motors.

@Rocketct
Copy link
Contributor

Rocketct commented Aug 5, 2021

ok so this means that the problem doesn't exist

@aliphys
Copy link
Member Author

aliphys commented Aug 5, 2021

That is great @Rocketct 😃 .
How much work would it take to to make the ArduinoMotorCarrier library compatible with the Portenta H7?

@Rocketct
Copy link
Contributor

Rocketct commented Aug 5, 2021

i no have idea, but before this we want that is compatible? what saw the project owner? have sense make it compatible?

@aentinger aentinger merged commit 8d009e9 into arduino-libraries:master Dec 30, 2022
@per1234 per1234 added type: enhancement Proposed improvement topic: documentation Related to documentation for the project labels Jan 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: documentation Related to documentation for the project type: enhancement Proposed improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants