Modify state management aberridg #45
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…te if the class and ID match, rather than using flags that get set during the process and processUBX functions. Still some tidy-up required, but makes debugging and maintenance easier and helps prevents some race conditions/panics/crashes (due to writing data where it shouldn't be written)!
…e management out of the process and processUBX functions.
…ne much higher up the call chain
…ust downwards if you feel necessary.
|
Sincere thanks for submitting this Andrew (@aberridg ), |
PaulZC
merged commit 2d502ec
into
sparkfun:release_candidate_v2.1_modified_state_management
|
Hi Andrew (@aberridg ), |
|
Hi Paul,
Sounds like excellent progress!
I'm taking a look at ex. 19 right now. It's actually crashing my ESP32 with
a Guru Meditation error.
I'll check why it's failing and get back to you later on! I'll also look at
the other examples too :-)
Thanks,
Andrew
…On Thu, 1 Jul 2021 at 16:46, Paul ***@***.***> wrote:
Hi Andrew ***@***.*** <https://github.com/aberridg> ),
I've released your SPI code as version 2.0.8 of the library. Thanks again
for that.
I've also successfully merged v2.0.8 into the
release_candidate_v2.1_modified_state_management branch.
The good news is that I can get simple examples like Example3 to work OK.
The bad news is that examples like 19 (dynamic model) and 20 (custom
packet) don't work... I'm using I2C but I suspect the same will be true on
SPI.
In both examples, setI2COutput is failing and I don't currently know why.
In example 19, setDynamicModel fails in the same way. I'm wondering if
this is due to the changes you made to packetCfg and packetBuf?
Example 20 uses a custom ubxPacket created in the example code. I suspect
this one is failing because it looks like you've passed &packetCfg to
processUBX instead of incomingUBX from the code above? The code change is
here
<a92139c#diff-a127a9ca18073af471d3390aa8af066dfad1fee41cb0d38084f925a665183653L1318>
and here
<a92139c#diff-a127a9ca18073af471d3390aa8af066dfad1fee41cb0d38084f925a665183653R1311>
.
When you have time, can you see if you can get these two examples going?
Sincere thanks,
Paul
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#45 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AHLJITIJ64XZ2UV25QN7M4TTVSEU3ANCNFSM47MN4NKQ>
.
--
*Andrew Berridge - *TIBCO Data Science | *M*: +44 77 89764017 | *E:*
***@***.*** | www.tibco.com
|
|
Oops... Guru caused by forgetting to call begin()!
I can see it failing elsewhere. Looking into it.
Andrew
…On Thu, 1 Jul 2021 at 19:36, Andrew Berridge ***@***.***> wrote:
Hi Paul,
Sounds like excellent progress!
I'm taking a look at ex. 19 right now. It's actually crashing my ESP32
with a Guru Meditation error.
I'll check why it's failing and get back to you later on! I'll also look
at the other examples too :-)
Thanks,
Andrew
On Thu, 1 Jul 2021 at 16:46, Paul ***@***.***> wrote:
> Hi Andrew ***@***.*** <https://github.com/aberridg> ),
> I've released your SPI code as version 2.0.8 of the library. Thanks again
> for that.
> I've also successfully merged v2.0.8 into the
> release_candidate_v2.1_modified_state_management branch.
> The good news is that I can get simple examples like Example3 to work OK.
> The bad news is that examples like 19 (dynamic model) and 20 (custom
> packet) don't work... I'm using I2C but I suspect the same will be true on
> SPI.
> In both examples, setI2COutput is failing and I don't currently know why.
> In example 19, setDynamicModel fails in the same way. I'm wondering if
> this is due to the changes you made to packetCfg and packetBuf?
> Example 20 uses a custom ubxPacket created in the example code. I
> suspect this one is failing because it looks like you've passed
> &packetCfg to processUBX instead of incomingUBX from the code above? The
> code change is here
> <a92139c#diff-a127a9ca18073af471d3390aa8af066dfad1fee41cb0d38084f925a665183653L1318>
> and here
> <a92139c#diff-a127a9ca18073af471d3390aa8af066dfad1fee41cb0d38084f925a665183653R1311>
> .
> When you have time, can you see if you can get these two examples going?
> Sincere thanks,
> Paul
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#45 (comment)>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AHLJITIJ64XZ2UV25QN7M4TTVSEU3ANCNFSM47MN4NKQ>
> .
>
--
*Andrew Berridge - *TIBCO Data Science | *M*: +44 77 89764017 | *E:*
***@***.*** | www.tibco.com
--
*Andrew Berridge - *TIBCO Data Science | *M*: +44 77 89764017 | *E:*
***@***.*** | www.tibco.com
|
|
Hmm... it failed then worked the next time I reset the ESP32!
Weird. Will continue to look at it.
Andrew
…On Thu, 1 Jul 2021 at 20:04, Andrew Berridge ***@***.***> wrote:
Oops... Guru caused by forgetting to call begin()!
I can see it failing elsewhere. Looking into it.
Andrew
On Thu, 1 Jul 2021 at 19:36, Andrew Berridge ***@***.***> wrote:
> Hi Paul,
>
> Sounds like excellent progress!
>
> I'm taking a look at ex. 19 right now. It's actually crashing my ESP32
> with a Guru Meditation error.
>
> I'll check why it's failing and get back to you later on! I'll also look
> at the other examples too :-)
>
> Thanks,
>
> Andrew
>
> On Thu, 1 Jul 2021 at 16:46, Paul ***@***.***> wrote:
>
>> Hi Andrew ***@***.*** <https://github.com/aberridg> ),
>> I've released your SPI code as version 2.0.8 of the library. Thanks
>> again for that.
>> I've also successfully merged v2.0.8 into the
>> release_candidate_v2.1_modified_state_management branch.
>> The good news is that I can get simple examples like Example3 to work OK.
>> The bad news is that examples like 19 (dynamic model) and 20 (custom
>> packet) don't work... I'm using I2C but I suspect the same will be true on
>> SPI.
>> In both examples, setI2COutput is failing and I don't currently know
>> why.
>> In example 19, setDynamicModel fails in the same way. I'm wondering if
>> this is due to the changes you made to packetCfg and packetBuf?
>> Example 20 uses a custom ubxPacket created in the example code. I
>> suspect this one is failing because it looks like you've passed
>> &packetCfg to processUBX instead of incomingUBX from the code above?
>> The code change is here
>> <a92139c#diff-a127a9ca18073af471d3390aa8af066dfad1fee41cb0d38084f925a665183653L1318>
>> and here
>> <a92139c#diff-a127a9ca18073af471d3390aa8af066dfad1fee41cb0d38084f925a665183653R1311>
>> .
>> When you have time, can you see if you can get these two examples going?
>> Sincere thanks,
>> Paul
>>
>> —
>> You are receiving this because you were mentioned.
>> Reply to this email directly, view it on GitHub
>> <#45 (comment)>,
>> or unsubscribe
>> <https://github.com/notifications/unsubscribe-auth/AHLJITIJ64XZ2UV25QN7M4TTVSEU3ANCNFSM47MN4NKQ>
>> .
>>
>
>
> --
>
> *Andrew Berridge - *TIBCO Data Science | *M*: +44 77 89764017 | *E:*
> ***@***.*** | www.tibco.com
>
>
>
--
*Andrew Berridge - *TIBCO Data Science | *M*: +44 77 89764017 | *E:*
***@***.*** | www.tibco.com
--
*Andrew Berridge - *TIBCO Data Science | *M*: +44 77 89764017 | *E:*
***@***.*** | www.tibco.com
|
|
Aha! I got it...
I added another parameter to sendCommand when an ACK only is expected to
not wait for a data packet. Easy fix...
e.g.: change:
return ((sendCommand(&packetCfg, maxWait)) == SFE_UBLOX_STATUS_DATA_SENT);
// We are only expecting an ACK
to:
return ((sendCommand(&packetCfg, maxWait, true)) ==
SFE_UBLOX_STATUS_DATA_SENT); // We are only expecting an ACK
This needs changing for all such commands where only an ack is expected.
It's a safer way to detect exactly what we want. Means that we can detect
either a data packet and an ack or just an ack.
I'll change all the parts of the code where this happens. Not sure why this
didn't make it into the final merge as I'm sure I changed it!
Andrew
…On Thu, 1 Jul 2021 at 20:07, Andrew Berridge ***@***.***> wrote:
Hmm... it failed then worked the next time I reset the ESP32!
Weird. Will continue to look at it.
Andrew
On Thu, 1 Jul 2021 at 20:04, Andrew Berridge ***@***.***> wrote:
> Oops... Guru caused by forgetting to call begin()!
>
> I can see it failing elsewhere. Looking into it.
>
> Andrew
>
> On Thu, 1 Jul 2021 at 19:36, Andrew Berridge ***@***.***> wrote:
>
>> Hi Paul,
>>
>> Sounds like excellent progress!
>>
>> I'm taking a look at ex. 19 right now. It's actually crashing my ESP32
>> with a Guru Meditation error.
>>
>> I'll check why it's failing and get back to you later on! I'll also look
>> at the other examples too :-)
>>
>> Thanks,
>>
>> Andrew
>>
>> On Thu, 1 Jul 2021 at 16:46, Paul ***@***.***> wrote:
>>
>>> Hi Andrew ***@***.*** <https://github.com/aberridg> ),
>>> I've released your SPI code as version 2.0.8 of the library. Thanks
>>> again for that.
>>> I've also successfully merged v2.0.8 into the
>>> release_candidate_v2.1_modified_state_management branch.
>>> The good news is that I can get simple examples like Example3 to work
>>> OK.
>>> The bad news is that examples like 19 (dynamic model) and 20 (custom
>>> packet) don't work... I'm using I2C but I suspect the same will be true on
>>> SPI.
>>> In both examples, setI2COutput is failing and I don't currently know
>>> why.
>>> In example 19, setDynamicModel fails in the same way. I'm wondering if
>>> this is due to the changes you made to packetCfg and packetBuf?
>>> Example 20 uses a custom ubxPacket created in the example code. I
>>> suspect this one is failing because it looks like you've passed
>>> &packetCfg to processUBX instead of incomingUBX from the code above?
>>> The code change is here
>>> <a92139c#diff-a127a9ca18073af471d3390aa8af066dfad1fee41cb0d38084f925a665183653L1318>
>>> and here
>>> <a92139c#diff-a127a9ca18073af471d3390aa8af066dfad1fee41cb0d38084f925a665183653R1311>
>>> .
>>> When you have time, can you see if you can get these two examples going?
>>> Sincere thanks,
>>> Paul
>>>
>>> —
>>> You are receiving this because you were mentioned.
>>> Reply to this email directly, view it on GitHub
>>> <#45 (comment)>,
>>> or unsubscribe
>>> <https://github.com/notifications/unsubscribe-auth/AHLJITIJ64XZ2UV25QN7M4TTVSEU3ANCNFSM47MN4NKQ>
>>> .
>>>
>>
>>
>> --
>>
>> *Andrew Berridge - *TIBCO Data Science | *M*: +44 77 89764017 | *E:*
>> ***@***.*** | www.tibco.com
>>
>>
>>
>
>
> --
>
> *Andrew Berridge - *TIBCO Data Science | *M*: +44 77 89764017 | *E:*
> ***@***.*** | www.tibco.com
>
>
>
--
*Andrew Berridge - *TIBCO Data Science | *M*: +44 77 89764017 | *E:*
***@***.*** | www.tibco.com
--
*Andrew Berridge - *TIBCO Data Science | *M*: +44 77 89764017 | *E:*
***@***.*** | www.tibco.com
|
|
Hi Andrew, |
|
No worries! I'll fix it :-)
Andrew
…On Thu, 1 Jul 2021 at 20:49, Paul ***@***.***> wrote:
Hi Andrew,
It is entirely possible I’ve committed a heinous git merge atrocity. It
wouldn’t be my first. I remember seeing a lot of ‘, true’s being added.
Apologies in advance if I have short-circuited any…
Best wishes,
Paul
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#45 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AHLJITI3O7FR4YFIKCCZ2J3TVTBEDANCNFSM47MN4NKQ>
.
--
*Andrew Berridge - *TIBCO Data Science | *M*: +44 77 89764017 | *E:*
***@***.*** | www.tibco.com
|
|
Fixed ex 19.
20 tomorrow :-)
Andrew
…On Thu, 1 Jul 2021, 8:50 pm Andrew Berridge, ***@***.***> wrote:
No worries! I'll fix it :-)
Andrew
On Thu, 1 Jul 2021 at 20:49, Paul ***@***.***> wrote:
> Hi Andrew,
> It is entirely possible I’ve committed a heinous git merge atrocity. It
> wouldn’t be my first. I remember seeing a lot of ‘, true’s being added.
> Apologies in advance if I have short-circuited any…
> Best wishes,
> Paul
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#45 (comment)>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AHLJITI3O7FR4YFIKCCZ2J3TVTBEDANCNFSM47MN4NKQ>
> .
>
--
*Andrew Berridge - *TIBCO Data Science | *M*: +44 77 89764017 | *E:*
***@***.*** | www.tibco.com
|
|
Ex. 20 fixed, I think!
ubxPacket requires one fewer initializers as (in the lib) I've removed the
class and ID match status flag and made it into a member function.
Added true to sendCommand for expecting an ack only.
I recommend an addition to the README for the lib:
If you're sending custom commands to the Ublox device and you expect an ack
only, please add "true" as the final parameter to myGNSS.sendCommand. This
tells sendCommand to wait for an ack packet only, and not a data packet.
This should be the only breaking change to the public interface to the
library with the new state management code. I kept it as consistent as
possible!
Thanks,
Andrew
…On Thu, 1 Jul 2021 at 22:44, Andrew Berridge ***@***.***> wrote:
Fixed ex 19.
20 tomorrow :-)
Andrew
On Thu, 1 Jul 2021, 8:50 pm Andrew Berridge, ***@***.***> wrote:
> No worries! I'll fix it :-)
>
> Andrew
>
> On Thu, 1 Jul 2021 at 20:49, Paul ***@***.***> wrote:
>
>> Hi Andrew,
>> It is entirely possible I’ve committed a heinous git merge atrocity. It
>> wouldn’t be my first. I remember seeing a lot of ‘, true’s being added.
>> Apologies in advance if I have short-circuited any…
>> Best wishes,
>> Paul
>>
>> —
>> You are receiving this because you were mentioned.
>> Reply to this email directly, view it on GitHub
>> <#45 (comment)>,
>> or unsubscribe
>> <https://github.com/notifications/unsubscribe-auth/AHLJITI3O7FR4YFIKCCZ2J3TVTBEDANCNFSM47MN4NKQ>
>> .
>>
>
>
> --
>
> *Andrew Berridge - *TIBCO Data Science | *M*: +44 77 89764017 | *E:*
> ***@***.*** | www.tibco.com
>
>
>
--
*Andrew Berridge - *TIBCO Data Science | *M*: +44 77 89764017 | *E:*
***@***.*** | www.tibco.com
|
|
Oh well, not "true"... just true...
Thanks!
Andrew
…On Fri, 2 Jul 2021 at 10:17, Andrew Berridge ***@***.***> wrote:
Ex. 20 fixed, I think!
ubxPacket requires one fewer initializers as (in the lib) I've removed the
class and ID match status flag and made it into a member function.
Added true to sendCommand for expecting an ack only.
I recommend an addition to the README for the lib:
If you're sending custom commands to the Ublox device and you expect an
ack only, please add "true" as the final parameter to myGNSS.sendCommand.
This tells sendCommand to wait for an ack packet only, and not a data
packet.
This should be the only breaking change to the public interface to the
library with the new state management code. I kept it as consistent as
possible!
Thanks,
Andrew
On Thu, 1 Jul 2021 at 22:44, Andrew Berridge ***@***.***> wrote:
> Fixed ex 19.
>
> 20 tomorrow :-)
>
> Andrew
>
> On Thu, 1 Jul 2021, 8:50 pm Andrew Berridge, ***@***.***> wrote:
>
>> No worries! I'll fix it :-)
>>
>> Andrew
>>
>> On Thu, 1 Jul 2021 at 20:49, Paul ***@***.***> wrote:
>>
>>> Hi Andrew,
>>> It is entirely possible I’ve committed a heinous git merge atrocity. It
>>> wouldn’t be my first. I remember seeing a lot of ‘, true’s being added.
>>> Apologies in advance if I have short-circuited any…
>>> Best wishes,
>>> Paul
>>>
>>> —
>>> You are receiving this because you were mentioned.
>>> Reply to this email directly, view it on GitHub
>>> <#45 (comment)>,
>>> or unsubscribe
>>> <https://github.com/notifications/unsubscribe-auth/AHLJITI3O7FR4YFIKCCZ2J3TVTBEDANCNFSM47MN4NKQ>
>>> .
>>>
>>
>>
>> --
>>
>> *Andrew Berridge - *TIBCO Data Science | *M*: +44 77 89764017 | *E:*
>> ***@***.*** | www.tibco.com
>>
>>
>>
>
--
*Andrew Berridge - *TIBCO Data Science | *M*: +44 77 89764017 | *E:*
***@***.*** | www.tibco.com
--
*Andrew Berridge - *TIBCO Data Science | *M*: +44 77 89764017 | *E:*
***@***.*** | www.tibco.com
|
|
Thank again Andrew, I think you will need to open a new PR so I can merge your latest changes? Very best wishes, |
|
Thanks Paul!
I thought I had? Maybe I did it wrong. Will check later on
Andrew
…On Fri, 2 Jul 2021, 4:45 pm Paul, ***@***.***> wrote:
Thank again Andrew,
I think you will need to open a new PR so I can merge your latest changes?
Very best wishes,
Paul
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#45 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AHLJITLIFNHR3NKWSPDQRLDTVXNJZANCNFSM47MN4NKQ>
.
|
|
Oops. I must I have missed the email. I see the PR - thank you, |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As previously discussed - a rework of the state management code. Fewer flags, state checking moved to ubxPacket and removed from processUBX. Improved performance and reliability and maintainability. May still be scope to reduce the number of incoming UBX packet buffers. Contains all changes in add-spi-aberridg branch too :-)