greg/info-orbs
Watch
1
0
Fork
You've already forked info-orbs
 0
Code Issues 32 Pull requests 25 Projects Releases Packages Wiki Activity Actions

Pagination of securities #268

Merged
dgutzmann merged 9 commits from stock_mulit_page into dev 2025-03-15 21:11:58 +00:00
Conversation 16 Commits 9 Files changed 8 +71 -23
dgutzmann commented 2025-03-06 13:54:57 +00:00 (Migrated from github.com)
Copy link

Description

Allow more than 5 securities via pagination.

Changes

  • Increase task manager params to support more requests
    • There could me more optimization here to make it simpler and based on the total max requests needed
  • Moved the initialization bool to be a part of the stock data model instead of the stock widget
  • Fixed a check for m_stocks check to ensure that we don't try access past end of array
# Description Allow more than 5 securities via pagination. ## Changes * Increase task manager params to support more requests * There could me more optimization here to make it simpler and based on the total max requests needed * Moved the initialization bool to be a part of the stock data model instead of the stock widget * Fixed a check for m_stocks check to ensure that we don't try access past end of array
❤️ 1
lyricnz (Migrated from github.com) reviewed 2025-03-15 03:30:50 +00:00
firmware/src/widgets/stockwidget/StockDataModel.cpp
@ -142,0 +146,4 @@
StockDataModel &StockDataModel::setInitializationStatus(bool initialized) {
m_initialized = initialized;
return *this;
lyricnz (Migrated from github.com) commented 2025-03-15 03:30:50 +00:00
Copy link

Return value not needed/used. Yes, the other setters do the same (to permit chaining), but chaining is never used in this codebase from what I can see...

Return value not needed/used. Yes, the other setters do the same (to permit chaining), but chaining is never used in this codebase from what I can see...
lyricnz (Migrated from github.com) reviewed 2025-03-15 03:34:04 +00:00
firmware/src/widgets/stockwidget/StockWidget.cpp
lyricnz (Migrated from github.com) commented 2025-03-15 03:34:04 +00:00
Copy link

imho should be:
m_page = (m_page + 1) % m_pageCount;

imho should be: `m_page = (m_page + 1) % m_pageCount;`
lyricnz commented 2025-03-15 03:40:36 +00:00 (Migrated from github.com)
Copy link

config.h.template still says "Choose 5 securities..."

`config.h.template` still says "Choose 5 securities..."
lyricnz commented 2025-03-15 03:49:44 +00:00 (Migrated from github.com)
Copy link

When running this code with default config.h

#define STOCK_TICKER_LIST "BTC/USD,USD/CAD,XEQT,SPY,APC&country=Germany" // Choose 5 securities to track. You can track forex, crypto (symbol/USD) or stocks from any exchange (if one ticker is part of multiple exchanges you can add on "&country=Canada" to narrow down to your ticker)

the output said:

I: StockWidget::update - BTC/USD
I: StockWidget::update - USD/CAD
I: StockWidget::update - XEQT
I: StockWidget::update - SPY
I: StockWidget::update -^�?␟

But the actual HTTP requests looked OK

I: 🔵 Starting HTTP request for: https://api.twelvedata.com/quote?apikey=removed&symbol=BTC/USD
I: 🔵 Starting HTTP request for: https://api.twelvedata.com/quote?apikey=removed&symbol=USD/CAD
I: 🔵 Starting HTTP request for: https://api.twelvedata.com/quote?apikey=removed&symbol=XEQT
I: 🔵 Starting HTTP request for: https://api.twelvedata.com/quote?apikey=removed&symbol=SPY
I: 🔵 Starting HTTP request for: https://api.twelvedata.com/quote?apikey=removed&symbol=APC&country=Germany

Looks like some corruption / off-by-one? Digging

When running this code with default config.h ```cpp #define STOCK_TICKER_LIST "BTC/USD,USD/CAD,XEQT,SPY,APC&country=Germany" // Choose 5 securities to track. You can track forex, crypto (symbol/USD) or stocks from any exchange (if one ticker is part of multiple exchanges you can add on "&country=Canada" to narrow down to your ticker) ``` the output said: ``` I: StockWidget::update - BTC/USD I: StockWidget::update - USD/CAD I: StockWidget::update - XEQT I: StockWidget::update - SPY I: StockWidget::update -^�?␟ ``` But the actual HTTP requests looked OK ``` I: 🔵 Starting HTTP request for: https://api.twelvedata.com/quote?apikey=removed&symbol=BTC/USD I: 🔵 Starting HTTP request for: https://api.twelvedata.com/quote?apikey=removed&symbol=USD/CAD I: 🔵 Starting HTTP request for: https://api.twelvedata.com/quote?apikey=removed&symbol=XEQT I: 🔵 Starting HTTP request for: https://api.twelvedata.com/quote?apikey=removed&symbol=SPY I: 🔵 Starting HTTP request for: https://api.twelvedata.com/quote?apikey=removed&symbol=APC&country=Germany ``` Looks like some corruption / off-by-one? Digging
lyricnz commented 2025-03-15 04:12:59 +00:00 (Migrated from github.com)
Copy link

Clicking through to the stocks tag showed the same corruption:

: StockWidget::draw - BTC/USD
I: displayStock - BTC/USD ~ 84465.62
I: StockWidget::draw - USD/CAD
I: displayStock - USD/CAD ~ 1.44
I: StockWidget::draw - XEQT
I: displayStock - XEQT ~ 33.90
I: StockWidget::draw - SPY
I: displayStock - SPY ~ 562.81
I: StockWidget::draw - �>�?␟
I: displayStock - APC&country=Germany ~ 195.34
Clicking through to the stocks tag showed the same corruption: ``` : StockWidget::draw - BTC/USD I: displayStock - BTC/USD ~ 84465.62 I: StockWidget::draw - USD/CAD I: displayStock - USD/CAD ~ 1.44 I: StockWidget::draw - XEQT I: displayStock - XEQT ~ 33.90 I: StockWidget::draw - SPY I: displayStock - SPY ~ 562.81 I: StockWidget::draw - �>�?␟ I: displayStock - APC&country=Germany ~ 195.34 ```
lyricnz commented 2025-03-15 04:14:25 +00:00 (Migrated from github.com)
Copy link

By comparison:

void StockWidget::displayStock(int8_t displayIndex, StockDataModel &stock, uint32_t backgroundColor, uint32_t textColor) {
    Log.infoln("displayStock - %s ~ %s", stock.getSymbol().c_str(), stock.getCurrentPrice(2).c_str());

vs

            Log.infoln("StockWidget::draw - %s", m_stocks[i].getSymbol());

Looks like the latter is missing a c_str()

By comparison: ```cpp void StockWidget::displayStock(int8_t displayIndex, StockDataModel &stock, uint32_t backgroundColor, uint32_t textColor) { Log.infoln("displayStock - %s ~ %s", stock.getSymbol().c_str(), stock.getCurrentPrice(2).c_str()); ``` vs ```cpp Log.infoln("StockWidget::draw - %s", m_stocks[i].getSymbol()); ``` Looks like the latter is missing a `c_str()`
👍 1
lyricnz commented 2025-03-15 04:27:34 +00:00 (Migrated from github.com)
Copy link

Q: when the widget switches pages, can we just updated the orbs one-by-one, rather than clearing them all to black, and having them reappear over a second or so?

Edit: the (minor) problem with just removing the .clearAllScreens() call is when the "loading..." text is shown. The real displayStock() call includes its own clearScreen()

Q: when the widget switches pages, can we just updated the orbs one-by-one, rather than clearing them all to black, and having them reappear over a second or so? Edit: the (minor) problem with just removing the `.clearAllScreens()` call is when the "loading..." text is shown. The real `displayStock()` call includes its own `clearScreen()`
lyricnz (Migrated from github.com) reviewed 2025-03-15 04:51:14 +00:00
firmware/src/widgets/stockwidget/StockWidget.cpp
@ -157,6 +173,14 @@ void StockWidget::displayStock(int8_t displayIndex, StockDataModel &stock, uint3
m_manager.drawString(stock.getCurrencySymbol() + stock.getCurrentPrice(2), centre, 155, bigFontSize, Align::MiddleCenter);
lyricnz (Migrated from github.com) commented 2025-03-15 04:51:13 +00:00
Copy link

Too much noise

Too much noise
lyricnz (Migrated from github.com) reviewed 2025-03-15 04:51:40 +00:00
firmware/src/widgets/stockwidget/StockWidget.cpp
@ -8,24 +8,28 @@
StockWidget::StockWidget(ScreenManager &manager, ConfigManager &config) : Widget(manager, config) {
lyricnz (Migrated from github.com) commented 2025-03-15 04:51:34 +00:00
Copy link

Too much noise

Too much noise
lyricnz (Migrated from github.com) approved these changes 2025-03-15 05:09:14 +00:00
lyricnz (Migrated from github.com) left a comment
Copy link

LGTM (feel free to revert my changes if you want)

LGTM (feel free to revert my changes if you want)
dgutzmann (Migrated from github.com) reviewed 2025-03-15 14:55:52 +00:00
firmware/src/widgets/stockwidget/StockDataModel.cpp
@ -142,0 +146,4 @@
StockDataModel &StockDataModel::setInitializationStatus(bool initialized) {
m_initialized = initialized;
return *this;
dgutzmann (Migrated from github.com) commented 2025-03-15 14:55:52 +00:00
Copy link

Yep, was just trying to follow the pattern

Yep, was just trying to follow the pattern
dgutzmann commented 2025-03-15 15:08:49 +00:00 (Migrated from github.com)
Copy link

LGTM (feel free to revert my changes if you want)

Thanks for going through it in so much detail. Really appreciate it and love all the tweaks

> LGTM (feel free to revert my changes if you want) Thanks for going through it in so much detail. Really appreciate it and love all the tweaks
❤️ 1
lyricnz commented 2025-03-15 21:12:26 +00:00 (Migrated from github.com)
Copy link

Thanks for the PR. Merged

Thanks for the PR. Merged
lyricnz commented 2025-03-15 22:37:46 +00:00 (Migrated from github.com)
Copy link

Found a big where the screen flashes/refreshes even if <= 5 stocks. That should not happen.

Found a big where the screen flashes/refreshes even if <= 5 stocks. That should not happen.
lyricnz commented 2025-03-15 22:54:43 +00:00 (Migrated from github.com)
Copy link

Sorry - too many issues, going to need to revert this PR. Keep your branch open.
https://github.com/brettdottech/info-orbs/issues/290
https://github.com/brettdottech/info-orbs/issues/291 (two issues)

Sorry - too many issues, going to need to revert this PR. Keep your branch open. https://github.com/brettdottech/info-orbs/issues/290 https://github.com/brettdottech/info-orbs/issues/291 (two issues)
dgutzmann commented 2025-03-16 23:59:41 +00:00 (Migrated from github.com)
Copy link

Sorry - too many issues, going to need to revert this PR. Keep your branch open.
https://github.com/brettdottech/info-orbs/issues/290
https://github.com/brettdottech/info-orbs/issues/291 (two issues)

I see you fixed the issue I introduced with pagination causing flashing when there are 5 or less securities specified, but do you still want to revert this pr?

> Sorry - too many issues, going to need to revert this PR. Keep your branch open. > https://github.com/brettdottech/info-orbs/issues/290 > https://github.com/brettdottech/info-orbs/issues/291 (two issues) > > I see you fixed the issue I introduced with pagination causing flashing when there are 5 or less securities specified, but do you still want to revert this pr?
Sign in to join this conversation.
Reviewers
No reviewers
lyricnz
Labels
Clear labels   
bug
Something isn't working

  
conflicts

   
documentation
Improvements or additions to documentation

  
DO NOT MERGE
Work in progress not ready to be considered for merging

  
duplicate
This issue or pull request already exists

  
enhancement
New feature or request

  
good first issue
Good for newcomers

  
hardware
Problems or ideas about the hardware

  
help wanted
Extra attention is needed

  
invalid
This doesn't seem right

  
question
Further information is requested

  
wontfix
This will not be worked on

No labels
bug
conflicts
documentation
DO NOT MERGE
duplicate
enhancement
good first issue
hardware
help wanted
invalid
question
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
greg/info-orbs!268
No description provided.