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.

Sign up for GitHub

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

Jump to bottom

App config comments #316

Closed
wants to merge 11 commits into from
Closed

App config comments #316

wants to merge 11 commits into from

Conversation

DG12
Copy link
Contributor

@DG12 DG12 commented Dec 16, 2021

No description provided.

@ojousima-jenkins
Copy link
Collaborator

Can one of the admins verify this patch?

@ojousima
Copy link
Member

ok to test

ojousima
ojousima requested changes Dec 16, 2021
View reviewed changes
src/application_config/app_config.h Outdated Show resolved Hide resolved
src/application_config/app_config.h Show resolved Hide resolved
src/application_config/app_config.h Outdated Show resolved Hide resolved
src/application_config/app_config.h Outdated Show resolved Hide resolved
src/application_config/app_config.h Show resolved Hide resolved
src/application_config/app_config.h Outdated Show resolved Hide resolved
src/application_config/app_config.h Outdated Show resolved Hide resolved
src/application_config/app_config.h Outdated Show resolved Hide resolved
src/application_config/app_config.h Outdated Show resolved Hide resolved
test/test_app_log.c Show resolved Hide resolved
ojousima
ojousima requested changes Dec 17, 2021
View reviewed changes
src/application_config/app_config.h Outdated Show resolved Hide resolved
src/application_config/app_config.h Show resolved Hide resolved
src/application_config/app_config.h Outdated Show resolved Hide resolved
src/application_config/app_config.h Outdated Show resolved Hide resolved
src/application_config/app_config.h Outdated Show resolved Hide resolved
@DG12
Update config.h remove ifdef from required modules
d9563d2 
There are some compile time variables which are really required or the Ruuvi application won't run like
Scheduler,  RealTimeCLock and Radio.
@DG12 DG12 closed this Dec 28, 2021
@DG12
revise app_conf.h
e1afc1b 
Correct  parens and uppercase.
Remove "highlighting" line of // // 

Make calculation of APP_FLASH_PAGES consistent with loader commands, be conditional

i.e. :
#ifndef APP_FLASH_PAGES
APP_FLASH_PAGES    (0x15000/4096)-1       
#endif
@DG12 DG12 reopened this Dec 28, 2021
ojousima
ojousima requested changes Jan 3, 2022
View reviewed changes
Copy link
Member

@ojousima ojousima left a comment

Choose a reason for hiding this comment

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

This PR has a lot of changes other than comments. It changes (1U) and (0U) to (true), (false), removes #ifdefs which breaks application_modes overrides, removes test from test_app_log.c etc.

Please remove code changes if the goal is to add code comments

@DG12
Copy link
Contributor Author

DG12 commented Jan 3, 2022

I seem to have confused git ( and myself).
When you have a chance, can you suggest how to clean this up.

for my fork git says

  • This branch is 7 commits ahead, 4 commits behind

When I display this Pull Request it shows items 1-6 from previous Pull requests (including test_app_log which as we discussed :
ojousima closed this 18 days ago
)

  • added 7 commits 2 months ago
  1. SDK 15.3 soft linking should be symbolic
  2. Delete src.tar.gz
  3. Merge branch 'ruuvi:master' into master
  4. Merge branch 'ruuvi:master' into master (again)
  5. remove test of unused(removed )functions in test_app_log [[[ I thought I canceled this]]]
  6. de minimis
  7. add comments, calculate APP_FLASH_PAGES

@reviewpad reviewpad bot mentioned this pull request Mar 18, 2023
Merged
8 tasks
@ojousima
Copy link
Member

ojousima commented Oct 4, 2024

Closing as stale

@ojousima ojousima closed this Oct 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ojousima ojousima ojousima requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@DG12 @ojousima-jenkins @ojousima