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

st-info: Bad user experience due to too high clock frequency #990

Closed
oloftangrot opened this issue Jun 18, 2020 · 12 comments · Fixed by #1114
Closed

st-info: Bad user experience due to too high clock frequency #990

oloftangrot opened this issue Jun 18, 2020 · 12 comments · Fixed by #1114

Comments

@oloftangrot
Copy link

  • Programmer/board type: Every board
  • Programmer firmware version: Every firmware
  • Operating system and version: All operating systems
  • Stlink tools version and/or git commit hash: [HEAD]
  • Stlink commandline tool name: st-info
  • Target chip (and board if applicable): Verified on a STM32F108C

The st-info tool defaults to 1,8 MHz clock speed without telling the user about it or allowing the user to select a more adequate speed. Depending on cable lengths between the programmer and the target, and other environmental conditions this is not a stable speed for a tool that does not require high band width but rather should provide confidence in the tools.

I have made a local branch that fixes the problem by:

  • Defaults to a 100 kHz.
  • Always displays the used clock frequency
  • Allows the user to select frequency
  • Does not terminate without a clear error message if the user selects a frequency that is not supported.
  • Provides the user with information about valid clock frequencies.
  • Terminates on a unknown options and shows the option that caused termination.
@Nightwalker-87
Copy link
Member

@oloftangrot: If you desire such change to our toolset, you should put up a PR with these changes for review.

@oloftangrot
Copy link
Author

Don't ask me about my desires. It is an issue report. I forgot to mention selecting frequency by the abusing absolute temperature in Kelvin or prefix mili was not an option. I guess that would be on the issue report for st-flash tough.

@Nightwalker-87
Copy link
Member

Nightwalker-87 commented Jun 22, 2020

@oloftangrot: The point is if you have a fix - why don't you submit it as a proposal instead of opening an issue telling there is a solution?
I don't really understand the rest of your reply, sry.

@Nightwalker-87
Copy link
Member

@chenguokai @slyshykO: What shall we do with this? Should the documentation be improved?
I know that sometimes it does make sense to lower the clock speed (e.g. if the connection wire is of poor quality or very long).
However this is no general circumstance to be preset by a programming tool...

@chenguokai
Copy link
Collaborator

I would suggest adding a documentation for this.

BTW if I got it right, the default clock has become adjustable in v1.6.1.

@Nightwalker-87
Copy link
Member

Well there actually IS some documentation already, since your contribution in #985 .
The point of @oloftangrot seems to be that he expected it to be present in st-info.
I don't really understand why that should be necessary for this specific tool.

@Nightwalker-87 Nightwalker-87 changed the title [All devices]: Bad user experiance due to too high clock frequency in st-info st-info: Bad user experience due to too high clock frequency Jun 27, 2020
@oloftangrot
Copy link
Author

oloftangrot commented Jun 29, 2020

Added diff for my fix.

fix.diff.txt

@Nightwalker-87
Copy link
Member

@Ant-ON: What is your opinion on this?

Personally, I still don't see a general use case for this. Such long cable lengths that would possible require to reduce the clock speed down from the MHz range are not preferable in terms of primary line constants and/or external ingress. Also st-info is not a tool greatly interacting with the chipset - it only reads out MCU info.

@Ant-ON
Copy link
Collaborator

Ant-ON commented Mar 12, 2021

@Nightwalker-87 I think adding a frequency setting is a useful feature. But changes from "fix.diff.txt" need some work. At least the code style need change. It does not match the one used in the project.

@Nightwalker-87
Copy link
Member

@oloftangrot Please provide a PR instead of a diff.

@oloftangrot
Copy link
Author

I might have flushed that branch long time ago and don't really have time to check on it. And I would not know how to make a pull request even if I found it. So I guess it will be least work for all parts if you just apply my diff and commit it directy self.

@Nightwalker-87
Copy link
Member

Nightwalker-87 commented Mar 22, 2021

... and leave this ticket to automatic GitHub maintenance. ;-)

@stlink-org stlink-org locked as resolved and limited conversation to collaborators Mar 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.