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

Added NetworkClass::RSSI #152

Merged
merged 2 commits into from Mar 19, 2014
Merged

Added NetworkClass::RSSI #152

merged 2 commits into from Mar 19, 2014

Conversation

timothybrown
Copy link
Contributor

This function gives access to Relative Signal Strength Indicator (RSSI)
data from the CC3000’s scan table, which is refreshed every few
minutes. It returns signal strength in the range of -127 to 0 dB. Error Codes: 1 [CC3000 Error]; 2 [Timeout/No Data].

This function gives access to Relative Signal Strength Indicator (RSSI)
data from the CC3000’s scan table, which is refreshed every few
minutes. It returns signal strength in the range of -127 to 0 dB.
_functionTimeout = millis() + 1000;
_loopCount = 0;
_returnValue = 0;
while (millis() < _functionTimeout)
Copy link
Member

Choose a reason for hiding this comment

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

if _functionTimeout = millis() + 1000; is called right before it millis() wraps, while (millis() < _functionTimeout) exit early or immediately. It's once in a blue moon, but subtraction method is better way to CYA. while ((millis() - startTime) < 1000)

That said, I'm not sure it's going to work how you intended it to. If you break out of the inner while() loop because your wlan_scan_results_table[0] == 0 (results are corrupted), and you intend to re-run the function, wouldn't you want to also reset _loopCount = 0; maybe right above the inner while() loop? I don't understand why a 0 gets inserted into the results table, but if so and you don't reset _loopCount it's just going to continue on until it gets to 16.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would like to make sure that the workaround for borked responses from the
CC3000 is clearly delimited in comments or #ifdefs, so that when we get the
results to be reliable, we can remove that extra complexity.

On Mon, Mar 17, 2014 at 9:17 AM, Technobly notifications@github.com wrote:

In src/spark_wiring_network.cpp:

@@ -55,4 +55,38 @@ char* NetworkClass::SSID()
return (char *)ip_config.uaSSID;
}

+/* *********************************************** /
+/
* Network.RSSI() - @timothybrown - 2014.03.17 * /
+/
*********************************************** /
+/
---------------------------------------------------------------------------------------------------------------------------------------------- /
+/
This function returns the WiFi signal strength in -dB [-127 to 0]. Error Values: 1 [CC3000 Problem]; 2 [Function Timeout/Bad Data]. /
+/
It reads through all entires in the table, continuing even after finding the correct SSID; this prevents stale entries on the next call. /
+/
There is a one second timeout on the function for safety; this also forces the function to re-run if the results table is corrupt. /
+/
Note: The CC3000 only updates the RSSI status every few minutes, so this function doesn't need to be called constantly in the main loop. /
+/
---------------------------------------------------------------------------------------------------------------------------------------------- */
+int8_t NetworkClass::RSSI()
+{

  • _functionTimeout = millis() + 1000;
  • _loopCount = 0;
  • _returnValue = 0;
  • while (millis() < _functionTimeout)

if _functionTimeout = millis() + 1000; is called right before it millis()
wraps, while (millis() < _functionTimeout) exit early or immediately.
It's once in a blue moon, but subtraction method is better way to CYA. while
((millis() - startTime) < 1000)

That said, I'm not sure it's going to work how you intended it to. If you
break out of the inner while() loop because your wlan_scan_results_table[0]
== 0 (results are corrupted), and you intend to re-run the function,
wouldn't you want to also reset _loopCount = 0; maybe right above the
inner while() loop? I don't understand why a 0 gets inserted into the
results table, but if so and you don't reset _loopCount it's just going to
continue on until it gets to 16.

Reply to this email directly or view it on GitHubhttps://github.com//pull/152/files#r10657339
.

Andy

@timothybrown
Copy link
Contributor Author

@andyw-lala It's not really extra complexity as having a timeout is a good idea IMHO. (That's the only thing I added to catch the corrupt table issue is the 1 second while loop.)

@technobly I left the Core running last night and it called the function 50,000 times without failing to return a single result, but I do like the idea of changing the millis() code, just in case!

As for the loop count, that's a good point. The odd thing is it does seem to work somehow (when I had debug code in, I'd see it loop 20 to 30 times on occasion). Maybe I was resetting the loop count and it just got removed when I was cleaning the code or something. It is a mystery.

I'll just move the one at the start of the function into the while millis() loop. Sound good?

As for the reason the results table returns a zero count sometimes, I suspect it's happening during a refresh of the scan table. Unfortunately there's no flag or anything to tell us. So the only way around it is to assume a zero result is failure and to retry. (The original and still primary reason to check for the zero is to know when we've reached the last entry in the scan table. The first byte of each result lists the number of entries left, however we can't just stop once we find our SSID in the list (because then the next time the function was called we'd pick up in the list where we left off). Basically, you're supposed to read the entire table out for it to be fresh the next time, so that's why we break on zero.

It just so happens that when the weird error occurs, it lists zero entries. So enclosing the whole loop in another while loop with a timeout (which is good practice) makes sure we don't return bad data.

Like I said, I highly suspect we're getting no tables back when the CC3000 is doing a scan. I bet they're deleting the existing data, doing the scan (which takes a few ms) before writing the new table all at once. I doubt this will ever be fixed, but I'll make some more detailed comments at the end I of the function explaining why it's written that way. :)

@andyw-lala
Copy link
Contributor

I do plan to create a test case and push for a fix so that the API call is
reliable (e.g. always returns good data or a failure, not the current
behaviour) - so while I don't plan on holding my breath for this fix, I do
plan to get this fixed.

On Mon, Mar 17, 2014 at 9:57 AM, Timothy Brown notifications@github.comwrote:

@andyw-lala https://github.com/andyw-lala It's not really extra
complexity as having a timeout is a good idea IMHO. (That's the only thing
I added to catch the corrupt table issue is the 1 second while loop.)

@technobly https://github.com/technobly I left the Core running last
night and it called the function 50,000 times without failing to return a
single result, but I do like the idea of changing the millis() code, just
in case!

As for the loop count, that's a good point. The odd thing is it does seem
to work somehow (when I had debug code in, I'd see it loop 20 to 30 times
on occasion). Maybe I was resetting the loop count and it just got removed
when I was cleaning the code or something. It is a mystery.

I'll just move the one at the start of the function into the while
millis() loop. Sound good?

As for the reason the results table returns a zero count sometimes, I
suspect it's happening during a refresh of the scan table. Unfortunately
there's no flag or anything to tell us. So the only way around it is to
assume a zero result is failure and to retry. (The original and still
primary reason to check for the zero is to know when we've reached the last
entry in the scan table. The first byte of each result lists the number of
entries left, however we can't just stop once we find our SSID in the list
(because then the next time the function was called we'd pick up in the
list where we left off). Basically, you're supposed to read the entire
table
out for it to be fresh the next time, so that's why we break on
zero.

It just so happens that when the weird error occurs, it lists zero
entries. So enclosing the whole loop in another while loop with a timeout
(which is good practice) makes sure we don't return bad data.

Like I said, I highly suspect we're getting no tables back when the CC3000
is doing a scan. I bet they're deleting the existing data, doing the scan
(which takes a few ms) before writing the new table all at once. I doubt
this will ever be fixed, but I'll make some more detailed comments at the
end I of the function explaining why it's written that way. :)

Reply to this email directly or view it on GitHubhttps://github.com//pull/152#issuecomment-37824823
.

Andy

@timothybrown
Copy link
Contributor Author

Okay I'll add some comments explaining the bug, but even if it were fixed there wouldn't really need to be any changes made to this function as it'd still work the exact same way.

@technobly
Copy link
Member

Yes, sounds good Tim!

@timothybrown
Copy link
Contributor Author

Okay, I added your suggested changes @technobly and all seems to be well. After thinking about it, you are right on the money with the mills() thing. It only rolls over what, ever 50 days? I don't think a Spark Core could possibly even stay online that long at this point (LOL) but it's good future proofing and The Right Way�™ to do things. Also, I looked back through my revision history, and sure enough I deleted the second _loopCount quite awhile ago (before I even submitted the first pull request!) so, uh whoops!

I'll be updating my pull request shortly. =)

(By the way, I guess I didn't have to void that first request. I didn't realize pulls would auto-update from the branch they came from, neat! I spent years using CVS so I still get stuck in that mindset sometimes. GIT is lightyears better, let me tell you!)

@timothybrown
Copy link
Contributor Author

Okay, just pushed my changes. This should be ready go for merging into the master branch. =)

* @version V1.0.0
* @date 10-Nov-2013
* @author Satish Nair, Timothy Brown
* @version V1.0.3
Copy link
Member

Choose a reason for hiding this comment

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

I question changing the version... too tired right now to make a valid case why...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Force of habit I guess? I'm used to an over-all build numbering scheme with separate individual file versions and author lists. (IMHO it makes it super easy at a glance to determine if you're working on the latest update of a particular piece of the code who's worked on what and when.)

I can fix it. Like I said, force of habit. G

Copy link
Member

Choose a reason for hiding this comment

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

Ok coffee poured into eyes... still not helping.

That said, this revisioning didn't strike me as odd the first time you submitted it, but now that you've rev'd it a few times before this has even merged is not the way I typically revision a file. All of these edits should still be considered V1.0.1, but Spark may not even want to do that. I haven't seen them revisioning these files at all.

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Definitely keep the addition of your name to the authors list.
  2. Let's leave the version alone for now.

When I deploy firmware to the Sparkulator (just as soon as I can get through all/most of the outstanding pull requests and perform regression testing) I'll be calling the overall firmware version 0.2.0. Let's take the discussion of possible divergent file versions somewhere else—maybe an elite community thread.

@towynlin towynlin merged commit bc7c035 into particle-iot:master Mar 19, 2014
m-mcgowan pushed a commit that referenced this pull request Jul 28, 2018
Fix UART assert issue caused by repeated initialization of the serial…
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants