Notifications
Clear all
Topic starter
2025-03-09 2:01 am
In reply to post Introduce Yourself, I noticed a few issues after reading the code. The program description says the touch screen data is transmitted via WiFi. This is performed by transmitting a struct_message data object. For this discussion, the only fields of interest in the structure are
typedef struct {
//...
int Value_StringUP;
int Value_StringDOWN;
int Value_String1;
//...
} struct_message;
The OnDataRecv function handles receiving the message by copying the message to a local variable (myData1),
struct_message myData1;
//...
void OnDataRecv(
const esp_now_recv_info_t *info
, const uint8_t *incomingData
, int len
) {
//...
if (memcmp(mac, sender3_MAC, 6) == 0) {
memcpy(&myData1, incomingData, sizeof(myData1));
// ...
}
}
I assume the message contains only the changed values and not any previous values.
This all fine and good but at the end of the if statement OnDataRecv does something it shouldn't,
if (memcmp(mac, sender3_MAC, 6) == 0) {
// ...
motorRunningForward = false; // Reset motorRunningForward flag on new data received
motorRunningBackward = false; // Reset motorRunningBackward flag on new data received
}
These assignments perform a state change and that's not OnDataRecv's job. OnDataRecv just copies the incoming message; it doesn't handle the message. I'll come back to this later. The only other point to make is that OnDataRecv is a callback function that is called independently by the underlying OS.
... when I touch "1" and "UP" the unit will transmit via esp_WiFi, Value_String1 =1 and Value_StringUP = 1. When the motor receive that values it should soft-start and ramp up to speed, ...
The handling of messages is performed by the loop function and the first message is defined when Value_StringUP = 1 and Value_String1 =1.
void loop() {
if (myData1.Value_String1 == 1 && myData1.Value_StringUP == 1) {
//...
}
else if ( /* ... */) { // another message
//...
}
else { // default message
//...
}
delay(100); // Adjust delay as needed
}
If the current message isn''t the first defined message then the code goes to the first else clause and checks for another message. At ther end of these else clauses the default message handling is performed. Finally, a delay is executed for each iteration of the loop function.
... and when Value_StringUP = 0 the motor needs to stop without ramping down.
This defines the second message, Value_StringUP = 0 and Value_String1 = 1, and should be handled by the else clause for "another message." As you may suspect, it doesn't,
else if (myData1.Value_String1 == 1 && myData1.Value_StringDOWN == 1) {
//...
}
This is the third message described by
... when Value_String1 = 1 and Value_StringDOWN = 1 then motors to run back without soft-start ...
The code is missing the else clause for the second message
So when the second message is recieved the default else clause is performed, which is intended to turn the motor off,
//...
else {
if (motorRunningForward || motorRunningBackward) {
Serial.println("MotorA stopping");
motorA.motorStop();
motorRunningForward = false; // Reset motorRunningForward flag
motorRunningBackward = false; // Reset motorRunningBackward flag
}
}
This brings us back to the state variables motorRunningForward and motorRunningBackward. Turning the motor off depends upon the state of motorRunningForward and motorRunningBackward. If either is 1 (true) then the motor is running and is turned off via motorA.motorStop(). (It's unclear what happens if motorA.motorStop(). is called when the motor isn't running.)
In any event, after the call to motorA.motorStop() is when the state variabkes are both reset to false. This is why having OnRecvData reset them is a problem. When the first message is processed, the code sets the values
void loop() {
// MotorA runs forward until condition changes
if (myData1.Value_String1 == 1 && myData1.Value_StringUP == 1) {
if (!motorRunningForward) {
// ...
motorRunningForward = true; // Set motorRunningForward flag
motorRunningBackward = false; // Ensure backward flag is reset
} else {
// ...
}
}
// ...
The code is now waiting for the second message (which isn't processed) to be received. But when OnRecvData gets the second message, it resets motorRunningForward, which means the code forgets the motor is running! This is not good.
Aside minutia: there's a whole different aspect to this that depends upon when OnRecvData is called, i.e., can it interrupt the loop function. This sets up a race conditon that highlights the problem of having OnRecvData modify the state variables.
I think you want to have the loop function handle each mesage. I count 4 messages: and the default message handler
Value_StringUP = 1 and Value_String1 = 1.
Value_StringUP = 0 and Value_String1 = 1.
Value_StringDOWN = 1 and Value_String1 = 1.
Value_StringDOWN = 0 and Value_String1 = 1.
And don't allow OnRecvData to modify the state variables.
You might want to rethink the message conditions. Notice that Value_String1 = 1 is always 1.
The one who has the most fun, wins!
2025-03-11 9:20 am
Hi TFMcCarthy. Thanks for your assistance. I done what you tell me and motors start and stop, however when starting up the motor start up and ramp up to speed, stop and ramp up again and keeps on starting, ramp up and stop. When I change the conditions to stop, the motor only stop when motor reaches speed. I need the motor to ramp up and while conditions are met needs to keep running.
void loop() {
// MotorA runs forward first message
if (myData1.Value_String1 == 1 && myData1.Value_StringUP == 1) {
Serial.println("MotorA Running Forward");
for (int pwm = 0; pwm <= 255; pwm++) { // ramp up forward.
motorA.motorGo(pwm);
motorRunningForward = true;
motorRunningBackward = false;
delay(10);
}
}
// MotorA runs backward second message
else if (myData1.Value_String1 == 1 && myData1.Value_StringDOWN == 1) {
Serial.println("MotorA Running Backwards");
for (int pwm = 0; pwm <= 250; pwm++) { // ramp up backward
motorA.motorGo(-pwm);
motorRunningForward = false;
motorRunningBackward = true;
}
}
// Stop the motor default message
else if (myData1.Value_StringUP == 0 && myData1.Value_StringDOWN == 0) {
Serial.println("MotorA stopping");
motorA.motorStop();
motorRunningForward = false;
motorRunningBackward = false;
}
2025-03-12 4:18 am
When I get this code working with one string I will include the rest. I have a total of 12 strings to manual tune. I will do the adjustments via 2 esp32's. The second part of the code is when I select Auto_Tune =1, I will feed 12 audio frequency's from a 12 coil pick-up and use that freq so that the guitar will tune itself. The frequency's will be done in code2 with a separate esp32 and send via esp-now.
Code 3 will receive data from the Foot pedals (Complete), and adjust the strings as required. I am using DC motors and the code will use hull sensors as position control. Code will use <PID_v1.h> as explained by Bill in the Workshop where I get most of my ideas from. I'm using Hull sensors in my first Electro Steel guitar and after 2 years the received signal is still 100% accurate.
In code 4, I will use the 12 coil pick-up to generate some effects, not sure what yet.
The audio between guitar and amp will be wireless as well.
The power to the guitar will run from the pedal bar via the 2 front legs to the main guitar. Complete wireless.
All hardware components are completed
Topic starter
2025-03-12 2:05 pm
done what you tell me and motors start and stop, however when starting up the motor start up and ramp up to speed, stop and ramp up again and keeps on starting, ramp up and stop
I think this is because you're processing the same message repeatedly until a new message comes in. You should only process the command once.
The one who has the most fun, wins!
2025-03-13 12:46 am
OK but how do I change the code to only process it once?
Topic starter
2025-03-13 3:17 am
OK but how do I change the code to only process it once?
Sorry. I've been distracted with another hobbyhorse, side project that's diverted me away from @robotBuilder's project that pulled me away from my robot arm project which interrupted ... uh, ... I've been busy.
You want to regulate the processing of commands by sequencing them. You can use a flag to indicate a new message has arrived and let onDataRecv update it
bool new_message{false};
void OnDataRecv( ... ) {
// ignore new messages until current message
// is processed
if (new_message) {
return;
}
//... get new message ...
new_message = true;
}
void loop()
{
// if no new message, do nothing
if (!new_message) {
return;
}
// ... process message ...
new_message = false; // allow new message
}
The one who has the most fun, wins!
2025-03-13 3:39 am
OK but how do I change the code to only process it once?
You want to regulate the processing of commands by sequencing them. You can use a flag to indicate a new message has arrived and let onDataRecv update it
It might easier just to reset the value of myData1.Value_String to zero at the end of loop(). That way it wouldn't match any of the if statements until a valid string number was loaded in OnDataRecv().
Also, as an aside, it would be smaller, cleaner and safer if you put all that activity in a subroutine passing the string number, up, down and motor objects in as parameters. Since all strings will presumably operate in exactly the same manner, you'd only need to test and modify the code in ONE place instead of having to duplicate it 12 times (one for each string).
The main loop would then just need to test if the string number matched '1' and then call the routine with the parameters associated with that string, else if string matched '2' then call ... and so on.
Anything seems possible when you don't know what you're talking about.
Topic starter
2025-03-13 4:19 am
It might easier just to reset the value of myData1.Value_String to zero at the end of loop(). That way it wouldn't match any of the if statements until a valid string number was loaded in OnDataRecv().
Did I get the variable name wrong? isn't it myData.Value_String1, i.e., one of 12 strings on the guitar? There are Value_String2-12 more and this command is just for one string. Of course, this is connected to a motor, not a string, and I'm guessing about additional commands, so I may be wrong. You would then change the 1 of 12 strings that was set.
I agree about using a subroutine to reduce the duplication.
The one who has the most fun, wins!
2025-03-13 4:37 am
Did I get the variable name wrong? isn't it myData.Value_String1, i.e., one of 12 strings on the guitar? T
No, I'm lazy and was just using enough of the name to make it obvious what part(s) I was using (e.g up and down).
I see that I was incorrect above in that the loop would need to test each string individually (as opposed to an if then else_if ...) because it's possible that more than one string may be specified by the input struct. Therefore, each string would need to be tested each time through the loop.
Your suggestion to use a 'new' flag to indicate that a new struct has been loaded would work better in that case, just set it to false at the end of the loop() and true when a new struct is received.
A question: what happens if up and down are both set to 1 ? Setting both to zero apparently stops the motor.
Anything seems possible when you don't know what you're talking about.
2025-03-13 4:42 am
Just keep in mind I'm only coding for a year or so 🤔 Meaning I have an idea what you are talking about but do not always have the knowledge to implement it. I will do the changes and test. Thanks so far, great to have the experienced to assist.
2025-03-13 5:12 am
Just keep in mind I'm only coding for a year or so 🤔 Meaning I have an idea what you are talking about but do not always have the knowledge to implement it.
Fair enough. Since you're relatively new, let me add a couple of things.
It seems in your struct that you're allowing all 12 strings to be tagged, but you have only one up and down indicator. Perhaps you intended all strings selected to be treated the same way, if you want each treated individually, then you'll need 12 ups and 12 downs.
Consider using an array instead of individual variables for the strings. So declare int strings[12] instead of String1, String2, ... String12. That way you can process them all with a for statement as in
for (int I=0;i<12;i++) {
If (strings[I]==1) {
// Do something clever
}
}
Also, if you do need separate up and down for each string, you can declare int ups[12] and int downs[12] and handle them the same way. A further improvement would be to #define or declare a const NUMSTRINGS=12 and use that instead of the literal 12 for the dimensions and the for loop limit.
Finally, consider creating a separate .h file in which you define the struct. You can then include it in both the sender and receiver sketches and that will guarantee that the structs never get out of synch.
Anything seems possible when you don't know what you're talking about.
2025-03-13 11:03 am
So TFMcCarthy I done your suggested changes.
Will, I added a ui.h where I put the receiving data as extern struct_message myData1;
If I declare int ups[12] then the structure will be a mismatch to the sender or do I have it wrong. Other than that I don't understand why I need to create a StringUP and StringDOWN for each string as it will over complicate the code as I need to create another 12 commands.
At the moment the structure do wat it should. The issue is that the commands repeat and I failed to get it out of the code.
I do however have issues with the speed the code react on the commands. I hope the structure in ui.h will sort that out, or is it something else?
I will send photos of how the screens are structured, which will make more sense.
Below the code. It does not get the structure data now. Did I miss something?
#include <FS_MX1508.h>
#include <WiFi.h>
#include <esp_now.h>
#include "ui.h"
#define pinA 23
#define pinB 22
#define pinC 21
#define pinD 19
MX1508 motorA(pinA, pinB);// default SLOW_DECAY (resolution 8 bits, frequency 1000Hz)
MX1508 motorB(pinC, pinD);
uint8_t sender2_MAC[] = {0xd8, 0x3b, 0xda, 0x85, 0x38, 0x5c}; // Screen myData1
bool motorRunningForward = false;
bool motorRunningBackward = false;
bool new_message{false};
void OnDataRecv(const esp_now_recv_info_t *info, const uint8_t *incomingData, int len) {
const uint8_t *mac = info->src_addr; // Extract sender's MAC address
if (new_message) {
return;
if (memcmp(mac, sender2_MAC, 6) == 0) {
}
memcpy(&myData1, incomingData, sizeof(myData1));
Serial.println("Data received from Sender 2 (myData1)");
Serial.print("StringDOWN: "); Serial.println(myData1.Value_StringDOWN);
Serial.print("StringUP: "); Serial.println(myData1.Value_StringUP);
Serial.print("String1: "); Serial.println(myData1.Value_String1);
Serial.print("String2: "); Serial.println(myData1.Value_String2);
Serial.print("String3: "); Serial.println(myData1.Value_String3);
Serial.print("String4: "); Serial.println(myData1.Value_String4);
Serial.print("String5: "); Serial.println(myData1.Value_String5);
Serial.print("String6: "); Serial.println(myData1.Value_String6);
}
new_message = true;
}
void setup() {
Serial.begin(115200);
WiFi.mode(WIFI_STA);
if (esp_now_init() != ESP_OK) {
Serial.println("ESP-NOW Init Failed!");
// Handle initialization failure (e.g., retry or enter a safe state)
return;
}
esp_now_register_recv_cb(OnDataRecv);
Serial.println("ESP-NOW Receiver Ready.");
}
void loop() {
// MotorA runs forward first message
if (!new_message) {
return;
}
if (myData1.Value_String1 == 1 && myData1.Value_StringUP == 1) {
Serial.println("MotorA Running Forward");
for (int pwm = 0; pwm <= 255; pwm++) { // ramp up forward.
motorA.motorGo(pwm);
motorRunningForward = true;
motorRunningBackward = false;
new_message = false; // allow new message
}
delay(10);
}
// }
// MotorA runs backward second message
else if (myData1.Value_String1 == 1 && myData1.Value_StringDOWN == 1) {
Serial.println("MotorA Running Backwards");
for (int pwm = 0; pwm <= 250; pwm++) { // ramp up backward
motorA.motorGo(-pwm);
motorRunningForward = false;
motorRunningBackward = true;
}
}
// Stop the motor default message
else if (myData1.Value_StringUP == 0 && myData1.Value_StringDOWN == 0) {
Serial.println("MotorA stopping");
motorA.motorStop();
motorRunningForward = false;
motorRunningBackward = false;
}
// MotorB runs forward first message
if (myData1.Value_String2 == 1 && myData1.Value_StringUP == 1) {
Serial.println("MotorB Running Forward");
for (int pwm = 0; pwm <= 255; pwm++) { // ramp up forward
motorB.motorGo(pwm);
motorRunningForward = true;
motorRunningBackward = false;
}
}
// MotorA runs backward second message
else if (myData1.Value_String2 == 1 && myData1.Value_StringDOWN == 1) {
Serial.println("MotorB Running Backwards");
for (int pwm = 0; pwm <= 200; pwm++) { // ramp up backward
motorB.motorGo(-pwm);
motorRunningForward = false;
motorRunningBackward = true;
}
}
// Stop the motor default message
else if (myData1.Value_StringUP == 0 && myData1.Value_StringDOWN == 0) {
Serial.println("MotorB stopping");
motorB.motorStop();
motorRunningForward = false;
motorRunningBackward = false;
}
delay(500); // Adjust delay as needed
}
Topic starter
2025-03-13 1:06 pm
... because it's possible that more than one string may be specified by the input struct.
I think this "hits the nail on the head." The content of the message needs further probing to understand it's purpose. Unfortunately, I'm not the right person to do that probing as I have no musical talent what-so-ever. I'd be lying to say otherwise and guessing about the best means to represent the data.
A question: what happens if up and down are both set to 1 ? Setting both to zero apparently stops the motor.
Case in point. I would think that such a message was ambiguous. As to what happens, the current implementation of the code uses test states that are exclusive of all other states and their test order defines their priority. First is StringUP+ON, then StringUP+OFF, then StringDOWN+ON, and then StringDOWN+OFF, So, if both StringUP and StringDOWN are set to 1 only StringUP is executed due to higher priority. This is similar to mathematical expression order of operations: multiplication and division followed by addition and subtraction.
However, I know that two musical notes can be played at once. How is that represented in the message? I dunno. Also, mechanically, I think the notes are played individually (or discretely) but very close together. Also, there the foot pedals. (OK, OK. Over my pay grade.)
So, I think the intent of the message should be clarified along with how the content of the message achieves that intent. What is the message intended to convey? That should clarify if messages contain a single or multiple actions. That should help with the message layout.
I'm strung out for time, and I don't think I can offer much more in terms of musical insight. I can only listen to music, not play it.
The one who has the most fun, wins!
2025-03-13 4:37 pm
The idea is to include the ui.h definition of the struct in BOTH the sender and receiver sketch, so there won't be a mismatch (because both are now guaranteed to be using the same variables in the same place).
Do you actually intend to tune all 12 strings (or partitions of them) up or down at the same time ? Or do you just handle one string up/down at a time ?
If you make both the motors and strings into an array, you can reduce your loop to something like ...
void loop() {
for (i = 0, i < 12; i++) { // For each string
if (strings[i] == 1) { // If flagged, then tune this string
Serial.print("Motor["); // Print prefix
Serial.print(i);
Serial.print("] ");
tune(myData1.Value_StringUP, myData1.Value_StringGOWN, motor[i]);
}
strings[i] = 0; // Disable string until new command
}
}
and perform all of the functionality in a subroutine such as ...
void tune(int up, int down, MX1508 &motor) {
if (up == 1) {
Serial.println("is running forward");
for (int pwm = 0; pwm <= 255; pwm++) { // ramp up forward.
motor.motorGo(pwm);
motorRunningForward = true;
motorRunningBackward = false;
delay(10);
}
} else if (down == 1) {
Serial.println("is running backward");
for (int pwm = 0; pwm <= 250; pwm++) { // ramp up backward
motor.motorGo(-pwm);
motorRunningForward = false;
motorRunningBackward = true;
}
}
// Stop the motor default message
else if (up == 0 && down == 0) {
Serial.println("is stopping");
motor.motorStop();
motorRunningForward = false;
motorRunningBackward = false;
} else {
Serial.println("iwas not unhandled");
}
}
Note that I have not tested this code, it is intended as an illustration, not a finished product.
Note that this should work as-is for all 12 strings, you just need to add the motor definitions as you go.
Anything seems possible when you don't know what you're talking about.
2025-03-13 11:28 pm
As can be seen on the touch screens when touching the "Manual Tune" it open the Manual screen. I can select multiple strings but generally I will only need to do one string at a time. This tuning will be used to wind new strings onto the guitar, and some guitarists like to off-tune the guitar for certain songs, so that is the main purpose of the Manual Tune. ONE STRING AT A TIME /ONE MOTOR AT A TIME.
Then you can see on the main screen there's an Auto Tune. That will be the next code I mentioned before. In that code the string Auto calibration will tune multiple strings at a time. It will use Frequency inputs to adjust strings.
if (frequency < 435) {
// Ramp up motor forward
....................
if (frequency > 435) {
// Ramp up motor backwards
Again, thanks for your help. I spend a lot of time to try to get the code working, with some success but because I don't have the coding background I do struggle.
The first Electro Steel Guitar I build was the first globally. In that guitar I used servo motors to do the string forward and backwards. As it was a simple circuit it didn't take long. On this current build I used stepper motors before but abandoned ship as I could not get the steppers to change the strings quick enough and it kept on losing steps.
Page 1 / 2
Next