Skip to content

Cellular : add modem version in mbed trace #12339

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
Merged
merged 1 commit into from
Feb 13, 2020
Merged

Cellular : add modem version in mbed trace #12339

merged 1 commit into from
Feb 13, 2020
+32 −2

Conversation

jeromecoutant
Copy link
Collaborator

Summary of changes

It could be useful to get in logs modem module version.

@LMESTM
@AriParkkila

Impact of changes

No impact with default compilation

Example with STMOD_CELLULAR when mbed-trace is enabled:

[INFO][CELL]: Modem manufacturer: Quectel
[INFO][CELL]: Modem model: BG96
[INFO][CELL]: Modem revision: BG96MAR02A07M1G

Migration actions required

Documentation

Pull request type 

[x] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results 

[] No Tests required for this change (E.g docs only update)
[x] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers

@ciarmcom ciarmcom requested review from a team January 30, 2020 16:00
@ciarmcom
Copy link
Member

@jeromecoutant, thank you for your changes.
@ARMmbed/mbed-os-wan @ARMmbed/mbed-os-maintainers please review.

jarvte
jarvte reviewed Jan 31, 2020
View reviewed changes
features/cellular/framework/device/CellularStateMachine.cpp Outdated
@@ -373,6 +374,22 @@ void CellularStateMachine::state_device_ready()
}
}
}

#if MBED_CONF_MBED_TRACE_ENABLE
CellularInformation *info = _cellularDevice.open_information();
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be done only when moving to next state (STATE_SIM_PIN)? We might be restarting/powering or failing to communicate with the modem.

Copy link

Choose a reason for hiding this comment

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

Yes, this should probably be just before if (device_ready()) at line 364.

AriParkkila
AriParkkila reviewed Jan 31, 2020
View reviewed changes
features/cellular/framework/device/CellularStateMachine.cpp Outdated

#if MBED_CONF_MBED_TRACE_ENABLE
CellularInformation *info = _cellularDevice.open_information();
char buf[80]; // this should actually be 2048, but then you should use heap memory or likely run out of the stack
Copy link

Choose a reason for hiding this comment

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

This could be something like:

char *buf = new (std::nothrow) char[2048]; // size from 3GPP TS 27.007
if (buf) {
  if (info->get_manufacturer..
  ...
  delete[] buf;
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

AriParkkila
AriParkkila suggested changes Feb 3, 2020
View reviewed changes
Copy link

@AriParkkila AriParkkila left a comment

Choose a reason for hiding this comment

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

@jeromecoutant Can you change this to use heap memory with new/delete?

APN and registration prints seems to duplicate what you see with the current traces and AT-debug enabled so they are probably not needed here?

@jeromecoutant jeromecoutant mentioned this pull request Feb 5, 2020
Closed
@0xc0170 0xc0170 added the release-version: 6.0.0-alpha-2 Second pre-release version of 6.0.0 label Feb 7, 2020
@mergify mergify bot added needs: CI and removed needs: work labels Feb 7, 2020
@0xc0170
Copy link
Contributor

0xc0170 commented Feb 7, 2020

CI started

@mbed-ci
Copy link

mbed-ci commented Feb 7, 2020

Test run: FAILED

Summary: 3 of 4 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-IAR
  • jenkins-ci/mbed-os-ci_build-GCC_ARM
  • jenkins-ci/mbed-os-ci_build-ARM

@0xc0170
Copy link
Contributor

0xc0170 commented Feb 10, 2020

Internal CI error, restarted

@mbed-ci
Copy link

mbed-ci commented Feb 10, 2020

Test run: FAILED

Summary: 1 of 11 test jobs failed
Build number : 2
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_greentea-test

@mergify mergify bot added needs: work and removed needs: CI labels Feb 10, 2020
@0xc0170
Copy link
Contributor

0xc0170 commented Feb 13, 2020

Test restarted

@0xc0170 0xc0170 merged commit d7f3341 into ARMmbed:master Feb 13, 2020
@mergify mergify bot removed the ready for merge label Feb 13, 2020
@jeromecoutant jeromecoutant deleted the PR_MODEMVERSION branch February 14, 2020 16:25
@adbridge
Copy link
Contributor

@AnttiKauppila @AriParkkila Doesn't this require updates to the documentation to detail the changes to displayed information ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jarvte jarvte jarvte left review comments

@0xc0170 0xc0170 0xc0170 approved these changes

@AnttiKauppila AnttiKauppila AnttiKauppila approved these changes

@AriParkkila AriParkkila AriParkkila approved these changes

Assignees
No one assigned
Labels
release-version: 6.0.0-alpha-2 Second pre-release version of 6.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@jeromecoutant @ciarmcom @0xc0170 @mbed-ci @adbridge @AnttiKauppila @AriParkkila @jarvte